Re: [quicwg/base-drafts] Don't arm the handshake timer if there's no data (#2590)

[ianswett <[e-mail]>]

ianswett commented on this pull request.

Thanks, I think this is closer.  I can also try to pull in all the changes from the other PR into this one if it makes this easier?

> @@ -458,11 +458,11 @@ sent, then no alarm should be armed until data has been received from the
 client.
 
 Because the server could be blocked until more packets are received, the client
-MUST start the crypto retransmission timer even if there is no unacknowledged
-CRYPTO data.  If the timer expires and the client has no CRYPTO data to
-retransmit and does not have Handshake keys, it SHOULD send an Initial packet in
-a UDP datagram of at least 1200 bytes.  If the client has Handshake keys, it
-SHOULD send a Handshake packet.
+MUST start the crypto retransmission timer until it has 1RTT keys, even if there

It's not just timeout(ie: it needs to be set after an ACK is received), but I'll try to clarify.

line 448: The definition of crypto packet is that it has a CRYPTO frame in it, so it's always ack-eliciting.

Retransmitting the crypto data would naturally update the timer(see previous sentence) and there would be unacknowledged crypto data in that case, so I'm not sure your extra clause is clarifying or potentially confusing?

> @@ -1114,7 +1114,8 @@ SetLossDetectionTimer():
     loss_detection_timer.update(loss_time)
     return
 
-  if (crypto packets are in flight):
+  if (crypto packets are in flight &&
+      crypto data to send):

That's in the the other PR: #2281 

> @@ -1151,7 +1152,8 @@ OnLossDetectionTimeout():
     DetectLostPackets(pn_space)
   // Retransmit crypto data if no packets were lost
   // and there are still crypto packets in flight.
-  else if (crypto packets are in flight):
+  else if (crypto packets are in flight &&
+           crypto data to send):
     // Crypto retransmission timeout.
     RetransmitUnackedCryptoData()
     crypto_count++

Also in the other PR.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2590#pullrequestreview-223286807