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 interval start time #585

Closed
martinthomson opened this issue Apr 29, 2020 · 1 comment · Fixed by #660
Closed

Persistent congestion interval start time #585

martinthomson opened this issue Apr 29, 2020 · 1 comment · Fixed by #660
Assignees
@agrover
Labels
bug Something isn't working task-medium Should take between one and three days
Projects
2H 2020 bug triage

Comments

@martinthomson
Copy link
Member

The spec is clear about this:

The congestion period is calculated as the time between the oldest and newest lost packets: ...

This is not what we are calculating; we calculate the time from the (previously) largest acknowledged packet. Is it a problem in that we might be hitting persistent congestion more easily?

It seems that it might be in that we might not have sent anything for a good long time between the largest acknowledged and the first packet that is lost. So it seems to me as though we need some different accounting in order to get the right start time.
@agrover agrover added this to Needs triage in 2H 2020 bug triage via automation Apr 30, 2020
@agrover agrover moved this from Needs triage to High priority in 2H 2020 bug triage Apr 30, 2020
@agrover agrover self-assigned this Apr 30, 2020
@agrover agrover added bug Something isn't working required-feature Is a new thing that needs to be added task-medium Should take between one and three days and removed required-feature Is a new thing that needs to be added labels May 1, 2020
@agrover agrover removed their assignment May 5, 2020
@agrover agrover mentioned this issue May 14, 2020
Closed
@agrover agrover self-assigned this May 20, 2020
agrover pushed a commit to agrover/neqo that referenced this issue May 20, 2020
Proposed fix to mozilla#585
3d42455 
Instead of tracking largest acked sent time, track oldest lost packet
time. Compare against most recent lost packet
@agrover agrover mentioned this issue May 20, 2020
Merged
@agrover agrover moved this from High priority to Medium Priority in 2H 2020 bug triage May 20, 2020
@agrover
Copy link
Contributor

agrover commented May 20, 2020

Making this medium priority. This is not critical for Milestone 2 (enabling in Nightly) -- Quiche doesn't even calculate PC.

agrover pushed a commit to agrover/neqo that referenced this issue May 20, 2020
Proposed fix to mozilla#585
e4caf0b 
As well as tracking largest acked sent time, track oldest lost packet
time. This is reset if largest acked sent time changes.
@agrover agrover mentioned this issue May 21, 2020
Closed
@mergify mergify bot closed this as completed in #660 May 25, 2020
2H 2020 bug triage automation moved this from Medium Priority to Closed May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@agrover agrover

Labels
bug Something isn't working task-medium Should take between one and three days
Projects
2H 2020 bug triage
Closed
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Proposed fix to #585 agrover/neqo
2 participants
@agrover @martinthomson