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

recovery pseudocode enters "recovery" after "slow start" #4045

Closed
kazu-yamamoto opened this issue Aug 25, 2020 · 6 comments · Fixed by #4055
Closed

recovery pseudocode enters "recovery" after "slow start" #4045

kazu-yamamoto opened this issue Aug 25, 2020 · 6 comments · Fixed by #4055
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@kazu-yamamoto
Copy link
Contributor

The spec says that "congestion avoidance" after "slow start". However, it seems to me that pseudocode enters "recovery" after "slow start".

When packets are lost, OnCongestionEvent is called:

OnCongestionEvent(sent_time):
  // Start a new congestion event if packet was sent after the
  // start of the previous congestion recovery period.
  if (!InCongestionRecovery(sent_time)):
    congestion_recovery_start_time = now()
    congestion_window *= kLossReductionFactor
    congestion_window = max(congestion_window, kMinimumWindow)
    ssthresh = congestion_window
    // A packet can be sent to speed up loss recovery.
    MaybeSendOnePacket()

This function sets congestion_recovery_start_time if the state is not "recovery", which means either "slow start" or "congestion avoidance". So, "recovery" starts after "slow start".

@larseggert larseggert added this to Triage in Late Stage Processing via automation Aug 25, 2020
@martinthomson martinthomson added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Aug 25, 2020
@project-bot project-bot bot moved this from Triage to Editorial Issues in Late Stage Processing Aug 25, 2020
@janaiyengar
Copy link
Contributor

@kazu-yamamoto : you are right; perhaps this could be made part of #3982?

@kazu-yamamoto
Copy link
Contributor Author

@janaiyengar Better definition of each state is welcome. But I'm talking about the pseudocode. My implementation based on the pseudocode enters "recovery" after "slow start". That's why I noticed this issue. We should fix the pseudocode somehow. (To fix this bug, I introduced a variable to hold the state explicitly in Haskell quic.)

martinthomson added a commit that referenced this issue Aug 26, 2020
This adds a state machine and makes further changes to the presentation
of the congestion control states in line with that presentation.  This
rolls in the normative changes from #3978.

Closes #4045.
@martinthomson
Copy link
Member

Entering recovery after slow start is correct. The language in the draft was confusing. Hopefully #4055 helps there.

@martinthomson martinthomson linked a pull request Aug 26, 2020 that will close this issue
@kazu-yamamoto
Copy link
Contributor Author

The language in the draft was confusing.

Indeed...

@kazu-yamamoto
Copy link
Contributor Author

After merging #4055, the pseudocode matches the language. Let's close this.

Late Stage Processing automation moved this from Editorial Issues to Issue Handled Aug 26, 2020
@martinthomson
Copy link
Member

Let's leave this open until #4055 merges then.

@martinthomson martinthomson reopened this Aug 26, 2020
Late Stage Processing automation moved this from Issue Handled to Triage Aug 26, 2020
@martinthomson martinthomson moved this from Triage to Editorial Issues in Late Stage Processing Aug 27, 2020
Late Stage Processing automation moved this from Editorial Issues to Issue Handled Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

4 participants