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

max_ack_delay is unknown when a new connection is established #2638

Closed
marten-seemann opened this issue Apr 22, 2019 · 10 comments · Fixed by #2646
Closed

max_ack_delay is unknown when a new connection is established #2638

marten-seemann opened this issue Apr 22, 2019 · 10 comments · Fixed by #2646
Labels
-recovery -transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@marten-seemann
Copy link
Contributor

The max_ack_delay is advertised in the transport parameters.
The recovery text assumes that this value is available right from the beginning of a connection.

@ianswett
Copy link
Contributor

I thought this sentence indicated it was 25ms until a value is received: "The maximum ack delay is communicated in the "max_ack_delay" transport parameter and the default value is 25ms.", but I agree it could be clearer, ie: "and the default value and the value prior to receiving the transport parameters is 25ms."?

As a related point, I think it turns out the max_ack_delay doesn't come into play until the transport params are available, because "crypto packets SHOULD use a very short ack delay, such as the local timer granularity."
https://tools.ietf.org/html/draft-ietf-quic-recovery-19#section-4.1

@marten-seemann
Copy link
Contributor Author

It's not used for sending, but we're using it when calculating the RTT.

@ianswett
Copy link
Contributor

Hmm, maybe we should default it to 0 before it's been received then?

Or should we just specify it's 25ms until transport params have been received?

@ianswett ianswett added the design An issue that affects the design of the protocol; resolution requires consensus. label Apr 23, 2019
@kazuho
Copy link
Member

kazuho commented Apr 23, 2019

I'd argue that we should use 0 until receiving TP.

We require endpoints to send ACKs for crypto packets immediately. That means that the ack-delay value found in the ACK packet is something that the peer could not control. It is my understanding that we consider these uncontrollable delays induced within an endpoint as part of RTT.

@janaiyengar
Copy link
Contributor

@marten-seemann : It's not about the RTT, it's about the PTO, or crypto timeout to be more precise. It's reasonable to continue using max_ack_delay for RTT computations, since it caps ack_delay (ack_delay should be 0 or something very small for those anyways). But when computing the crypto timeout, we don't want to include the max_ack_delay for the reasons you cite.

The draft currently does not have the max_ack_delay being used for the crypto retransmission timer, which was an oversight. We'll clean that up separately, but fortuitously, this means that there's no max_ack_delay in the pseudo_code or text to change.

I don't mind initializing it to 0 or any value, but it won't have any real effect on endpoint behavior based on the draft. max_ack_delay doesn't kick in until after the handshake is roughly complete.

@ianswett
Copy link
Contributor

The crypto retransmission timer doesn't use max_ack_delay intentionally, see: https://tools.ietf.org/html/draft-ietf-quic-recovery-20#section-4.1

"In order to quickly complete the handshake and avoid spurious
retransmissions due to crypto retransmission timeouts, crypto packets
SHOULD use a very short ack delay, such as the local timer
granularity. ACK frames SHOULD be sent immediately when the crypto
stack indicates all data for that packet number space has been
received.

@mikkelfj
Copy link
Contributor

Same applies to ack_delay_exponent

@marten-seemann
Copy link
Contributor Author

@mikkelfj

Same applies to ack_delay_exponent

Which why the spec says the following:

The default value is also used for ACK frames that are sent in Initial and Handshake packets.

@martinthomson
Copy link
Member

@mnot, the PR for this changed the transport doc. Can you check whether we need to run a consensus call?

@mnot mnot reopened this May 9, 2019
@ianswett ianswett added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Jun 12, 2019
@mnot mnot added this to Consensus Call issued in Late Stage Processing Jul 1, 2019
@mnot mnot added has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. and removed proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. labels Jul 8, 2019
@mnot mnot moved this from Consensus Call issued to Consensus Declared in Late Stage Processing Jul 8, 2019
@ianswett
Copy link
Contributor

The PR is already merged, so closing.

Late Stage Processing automation moved this from Consensus Declared to Text Incorporated Jul 10, 2019
@juliens juliens mentioned this issue Oct 12, 2019
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery -transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

7 participants