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

Split PTO calculation into a dedicated method #3681

Merged
merged 12 commits into from
May 31, 2020
61 changes: 31 additions & 30 deletions draft-ietf-quic-recovery.md
Original file line number Diff line number Diff line change
Expand Up @@ -1269,19 +1269,37 @@ timers wake up late. Timers set in the past fire immediately.
Pseudocode for SetLossDetectionTimer follows:

~~~
GetEarliestTimeAndSpace(times):
time = times[Initial]
GetLossTimeAndSpace():
time = loss_time[Initial]
space = Initial
for pn_space in [ Handshake, ApplicationData ]:
if (times[pn_space] != 0 &&
(time == 0 || times[pn_space] < time) &&
# Skip ApplicationData until handshake completion.
(pn_space != ApplicationData ||
IsHandshakeComplete()):
time = times[pn_space];
if (time == 0 || loss_time[pn_space] < time):
time = loss_times[pn_space];
ianswett marked this conversation as resolved.
Show resolved Hide resolved
space = pn_space
return time, space

GetPtoTimeAndSpace():
duration = smoothed_rtt + max(4 * rttvar, kGranularity) * (2 ^ pto_count)
# Arm PTO from now when there are no inflight packets
ianswett marked this conversation as resolved.
Show resolved Hide resolved
if (no in-flight packets):
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this is for the amplification protection, something's missing here. Perhaps this should be
if (is_client && no packets in flight && !PeerCompletedAddressValidation()).
Otherwise, you never get into the case below on line 1293.

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 added an assert that for !PeerCompletedAddressValidation().

I think the condition you're mentioning is:
"if (no in-flight packets in space):
continue;"

That's per PN-space, not for the connection as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind, I thought the code below to do the early return when no packets are in flight after address validation was removed. It's still there, so all good.

return now() + duration, Initial
ianswett marked this conversation as resolved.
Show resolved Hide resolved
timeout = infinite
pn_space = Initial
ianswett marked this conversation as resolved.
Show resolved Hide resolved
for space in [ Initial, Handshake, ApplicationData ]:
if (no in-flight packets in space):
continue;
if space == ApplicationData:
ianswett marked this conversation as resolved.
Show resolved Hide resolved
// Don't arm PTO for ApplicationData until handshake complete.
if (handshake is not complete):
return timeout, pn_space
// ApplicationData include the max_ack_delay in PTO.
ianswett marked this conversation as resolved.
Show resolved Hide resolved
duration += max_ack_delay * (2 ^ pto_count)

if (timeout > time_of_last_sent_ack_eliciting_packet[space] + duration):
timeout = time_of_last_sent_ack_eliciting_packet[space] + duration
pn_space = space
return timeout, space

PeerCompletedAddressValidation():
# Assume clients validate the server's address implicitly.
if (endpoint is server):
Expand All @@ -1293,7 +1311,7 @@ PeerCompletedAddressValidation():
has received HANDSHAKE_DONE

SetLossDetectionTimer():
earliest_loss_time, _ = GetEarliestTimeAndSpace(loss_time)
earliest_loss_time, _ = GetLossTimeAndSpace()
if (earliest_loss_time != 0):
// Time threshold loss detection.
loss_detection_timer.update(earliest_loss_time)
Expand All @@ -1313,23 +1331,8 @@ SetLossDetectionTimer():
return

// Determine which PN space to arm PTO for.
sent_time, pn_space = GetEarliestTimeAndSpace(
time_of_last_sent_ack_eliciting_packet)
// Don't arm PTO for ApplicationData until handshake complete.
if (pn_space == ApplicationData &&
handshake is not confirmed):
loss_detection_timer.cancel()
return
if (sent_time == 0):
assert(!PeerCompletedAddressValidation())
sent_time = now()

// Calculate PTO duration
timeout = smoothed_rtt + max(4 * rttvar, kGranularity) +
max_ack_delay
timeout = timeout * (2 ^ pto_count)

loss_detection_timer.update(sent_time + timeout)
timeout, _ = GetPtoTimeAndSpace()
loss_detection_timer.update(timeout)
~~~


Expand All @@ -1342,8 +1345,7 @@ Pseudocode for OnLossDetectionTimeout follows:

~~~
OnLossDetectionTimeout():
earliest_loss_time, pn_space =
GetEarliestTimeAndSpace(loss_time)
earliest_loss_time, pn_space = GetLossTimeAndSpace()
if (earliest_loss_time != 0):
// Time threshold loss Detection
lost_packets = DetectLostPackets(pn_space)
Expand All @@ -1355,8 +1357,7 @@ OnLossDetectionTimeout():
if (bytes_in_flight > 0):
// PTO. Send new data if available, else retransmit old data.
// If neither is available, send a single PING frame.
_, pn_space = GetEarliestTimeAndSpace(
time_of_last_sent_ack_eliciting_packet)
_, pn_space = GetPtoTimeAndSpace()
SendOneOrTwoAckElicitingPackets(pn_space)
else:
assert(endpoint is client without 1-RTT keys)
Expand Down