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

ACK of ACK are useful #2546

Closed
huitema opened this issue Mar 23, 2019 · 5 comments · Fixed by #2794
Closed

ACK of ACK are useful #2546

huitema opened this issue Mar 23, 2019 · 5 comments · Fixed by #2794
Assignees
Labels
-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

@huitema
Copy link
Contributor

huitema commented Mar 23, 2019

The transport spec says in section 13.1.1: "An endpoint MUST NOT send a packet containing only an ACK frame in response to a packet containing only ACK or PADDING frames, even if there are packet gaps which precede the received packet. This prevents an indefinite feedback loop of ACKs."

I argue that this MUST NOT should be changed to SHOULD NOT. Fist, the infinite feedback can be prevented if, for example, an implementation only sends an ACK for N packets received, so there is really no need to be that strict. Second, ACK of ACK are generally useful. They enable trimming the grow of ACK lists in a controlled way.

In fact, I found that problem in interop tests. Some implementations would send thousands of data packets without sending a single ACK of ACK. That means the sender is stuck with a large ACK dashboard. Yes, it could be trimmed with timers, but that's hardly a controlled process. I fixed this by sending an ACK + PING packet whenever there the unacked ACK list is too long, but that's a hack.

@ianswett
Copy link
Contributor

I'd argue that ACK+PING isn't a hack, but instead a simple way for the sender, whose state we're trying to limit, to balance the amount of state it stores vs the extra ACKs being received.

Getting the peer to implement the correct ACK behavior by sending a single extra byte is much simpler and more robust in my experience than relying on the receiver to implement an "ACK every 10/20/etc ACKs" policy in a way that every sender is happy with.

This could likely use more text and there's still an outstanding editorial issue to move most of the ACK sending text that's in recovery into transport, so it's all in one place, since I believe there are currently a few subtle inconsistencies.

@RyanTheOptimist
Copy link
Contributor

RyanTheOptimist commented Mar 24, 2019 via email

@mnot mnot added this to Triage in Late Stage Processing Mar 27, 2019
@janaiyengar
Copy link
Contributor

Agreed with @ianswett. Irrespective of what we recommend here, note that a sender of ACKs cannot assume what the peer will do -- when the peer will send an ACK for ACKs, I mean -- so, it will end up having to send a periodic PING anyways. Piggybacking a PING periodically (once an RTT, for example) is useful for RTT measurement as well, since ACK of ACKs do not get used for RTT (see #2568 for more discussion on this point).

@janaiyengar janaiyengar added the design An issue that affects the design of the protocol; resolution requires consensus. label Apr 3, 2019
@mnot mnot moved this from Triage to Design Issues in Late Stage Processing Apr 9, 2019
@igorlord
Copy link
Contributor

igorlord commented May 22, 2019

An alternative would be allowing sending ACKs-of-ACKs, if you are sending non-(ACK|PADDING) frames.

So, maybe: "An endpoint MUST NOT send a packet containing only ACK or PADDING frames in response to a packet containing only ACK or PADDING frames, if the previous packet it sent contained only ACK or PADDING frames."

@larseggert
Copy link
Member

As discussed in London, @ianswett to write a PR with some advisory text

ianswett added a commit that referenced this issue Jun 15, 2019
Fixes #2546 

If there's anything else discussed in London that I missed, please tell me.
@martinthomson martinthomson added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Jun 18, 2019
@mnot mnot moved this from Design Issues to Consensus Emerging in Late Stage Processing Jun 24, 2019
@mnot mnot moved this from Consensus Emerging 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
Late Stage Processing automation moved this from Consensus Declared to Text Incorporated Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-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.

8 participants