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.

Sign up for GitHub

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

Jump to bottom

persistent congestion can never happen #2208

Closed
marten-seemann opened this issue Dec 19, 2018 · 3 comments · Fixed by #2209
Closed

persistent congestion can never happen #2208

marten-seemann opened this issue Dec 19, 2018 · 3 comments · Fixed by #2209
Labels
-recovery 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 collapse of the congestion window (which used to happen after an RTO was verified) happens in CongestionEvent, if pto_count > kPersistentCongestionThreshold.
CongestionEvent is called from OnPacketsLost, which is called from DetectLostPackets, which in the case of a tail is only called when a new ACK is received from OnAckReceived. However, OnAckReceived resets the pto_count to 0 before calling DetectLostPackets, so the condition that leads to the collapse of the congestion window can never occur.
@marten-seemann marten-seemann mentioned this issue Dec 19, 2018
Merged
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Dec 19, 2018
@martinthomson
Copy link
Member

Marking as design, because it will affect interop if this is implemented as documented.

@marten-seemann
Copy link
Contributor Author

marten-seemann commented Dec 19, 2018 •
edited

Why is this design? The intention of the pseudo code was clearly that the congestion window is collapsed, and the text states this as well.

I also don’t think this will affect interop, other than the current draft will cause a little bit more congestion in the network.

@martinthomson
Copy link
Member

It's bug that probably needs to go into change logs, that's all. (We really need a 'bug' label for this sort of thing.)

@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
-recovery 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
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

run DetectLostPackets before resetting crypto_count and pto_count marten-seemann/base-drafts
3 participants
@martinthomson @mnot @marten-seemann