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

DetectLostPackets not called in anti-deadlock case #3298

Closed
larseggert opened this issue Dec 11, 2019 · 9 comments
Closed

DetectLostPackets not called in anti-deadlock case #3298

larseggert opened this issue Dec 11, 2019 · 9 comments

Comments

@larseggert
Copy link
Member

Why is DetectLostPackets only called for the time threshold loss case? Shouldn't it at least also be called for the anti-deadlock case?

@ianswett
Copy link
Contributor

DetectLostPackets() is called when an ACK is received that acknowledges new packets or the loss detection timer fires, which I believe lines up with the text.

Why would it be called in the anti-deadlock case? The only two loss triggers are packet threshold and time. Packet threshold requires a new ACK, and the loss time is only set/changed when a new ACK is received.

@larseggert
Copy link
Member Author

What if you get no ACKs during the handshake? You never declare packets lost. You still RTX them, but that seems suboptimal.

@RyanTheOptimist
Copy link
Contributor

RyanTheOptimist commented Dec 11, 2019 via email

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

It is by design that we don't mark things as lost until an ACK is received. That is, lack of feedback is not treated as loss to avoid undoing things later when the RTT may actually have changed. Receipt of an ACK is an important signal that the RTT is fine, and that it is actually likely that packets have been lost.

As Ryan asks, what happens that is suboptimal?

@larseggert
Copy link
Member Author

Well, if you have only a small amount of buffers (embedded device), and you RTX without freeing up the original TX (i.e., you con't declare it lost), you end up of of buffers pretty quickly.

Since anti-deadlock is only done during the handshake, there really isn't anything to undo?

@ianswett
Copy link
Contributor

I'm not sure what you're buffering model is, but I'd expect a RTX to only create a small amount of extra metadata, not a copy of the entire packet. How you do it depends upon whether you retransmit the payload of the original packet without changing it or use another approach, but it's intended to be low cost.

Is your concern on the client side or the server?

@larseggert
Copy link
Member Author

Since data is normally only RTX'ed after having been declared lost, I have (maybe prematurely) optimized for this case. So a series of RTXs without any incoming ACK will in fact occupy multiple buffers.

Maybe I just need to deal with this in my implementation and it doesn't require spec changes...

@janaiyengar
Copy link
Contributor

I suspect this is a special case. Our model has been to not mark anything as lost until we've received an ACK for something after it. I can see that you might have a bit of a cost in following this model with your implementation. Calling DetectLostPackets() there if you'd like as an optimization for your case, I don't think you break anything by doing it.

@larseggert
Copy link
Member Author

Will do, thanks

Late Stage Processing automation moved this from Triage to Text Incorporated Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

4 participants