Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handshake retransmissions #266

Merged
merged 8 commits into from Feb 17, 2017
Merged
43 changes: 22 additions & 21 deletions draft-ietf-quic-recovery.md
Expand Up @@ -201,27 +201,27 @@ Constants used in loss recovery and congestion control are based on a
combination of RFCs, papers, and common practice. Some may need to be changed
or negotiated in order to better suit a variety of environments.

* kMaxTLPs: 2
Maximum number of tail loss probes before an RTO fires.
* kMaxTLPs (default 2):
: Maximum number of tail loss probes before an RTO fires.

* kReorderingThreshold: 3
* kReorderingThreshold (default 3):
Maximum reordering in packet number space before FACK style loss detection
considers a packet lost.

* kTimeReorderingThreshold: 1/8
* kTimeReorderingThreshold (default 1/8):
Maximum reordering in time sapce before time based loss detection considers
a packet lost. In fraction of an RTT.

* kMinTLPTimeout: 10ms
* kMinTLPTimeout (default 10ms):
Minimum time in the future a tail loss probe alarm may be set for.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Editorial: this might look OK in markdown, but it turns into a real mess in the text version. I think that you want something like the following:

kMinTLPTimeout (default 10ms):
 : Minimum time in the future [...]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked it to say default as you said, but the extra : looked odd in markdown and text, so I didn't add it.

Is there an example formatting you were trying to achieve I should copy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra ':' is correct -- it's Markdown formatting for a definition list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the '*' at the start of the definition title line prevents kramdown from detecting it as a definition list. You will note its absence in my example.

You also need to reformat the whole list consistent or, as you say, it looks odd.


* kMinRTOTimeout: 200ms
* kMinRTOTimeout (default 200ms):
Minimum time in the future an RTO alarm may be set for.

* kDelayedAckTimeout: 25ms
* kDelayedAckTimeout (default 25ms):
The length of the peer's delayed ack timer.

* kDefaultInitialRtt: 200ms
* kDefaultInitialRtt (default 200ms):
The default RTT used before an RTT sample is taken.

## Variables of interest
Expand Down Expand Up @@ -366,21 +366,22 @@ below shows how the single timer is set based on the alarm mode.

### Handshake Packets

The initial client hello has no prior RTT sample. A client SHOULD remember
The initial flight has no prior RTT sample. A client SHOULD remember
the previous RTT it observed when resumption is attempted and use that for an
initial RTT value. If no previous RTT is available, the initial RTT defaults
to 200ms. Once an RTT measurement is taken, it MUST replace initial_rtt.

The client hello is retransmitted via a timer, similar to a tail loss probe, but
it is expected client hellos will be acked immediately or a handshake packet will
be sent immediately, so delayed acks do not need to be compensated for. As such,
it is always retransmitted at 2x the initial_rtt or smoothed_rtt.
Endpoints MUST retransmit handshake packets if not acknowledged(1) within a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (1) was my reference to a footnote in my comment. I think "not acknowledged" has to extend to be "not acknowledged and the next handshake message didn't arrive" but I'm not sure how to word that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I removed it and tweaked the text about exponential backoff again. Currently in the pseudocode(and the real code), all timers have their exponential backoff reset when an ack is received. The idea being, if you're getting some packets through, based the retransmission time on RTT. If you're not getting any data through, exponentially backoff because your RTT estimate may be wrong, and to avoid overloading the network.

time limit. This time limit will be the largest of twice the rtt value and
MinTLPTimeout. For each previous handshake timeout without receiving an
acknowledgement, the timeout doubles.

Server handshake packets may be sent based on a similar timer, or triggered by
receipt of another client hello.
When stateless rejects are in use, the connection is considered immediately
closed once a reject is sent, so no timer is set to retransmit the reject.

Version negotiation packets are always stateless, and MUST be sent once per
client hello, and may be sent in response to 0RTT packets.
per handshake packet that uses an unsupported QUIC version, and MAY be sent
in response to 0RTT packets.

(Add sections for early retransmit and TLP/RTO here)

Expand Down Expand Up @@ -447,10 +448,10 @@ supplied.

### Handshake Packets

The receiver MUST not trust an unencryped ack for encrypted packets. The receiver
MUST trust encrypted acks for unencrypted packets, however. Aside from this,
loss detection for handshake packets when an ack is processed is identical to
other packets.
The receiver MUST ignore unencrypted packets that ack encrypted packets.
The receiver MUST trust encrypted acks for unencrypted packets, however. Aside
from this, loss detection for handshake packets when an ack is processed is
identical to other packets.

### Psuedocode

Expand All @@ -471,7 +472,7 @@ Pseudocode for DetectLostPackets follows:
acked_packet.packet_number - reordering_threshold)
lost_packets.insert(unacked_packet.packet_number);
return lost_packets;
~~~
~~~

# Congestion Control

Expand Down