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

Clarify In-flight #2200

Closed
wants to merge 3 commits into from
Closed

Clarify In-flight #2200

wants to merge 3 commits into from

Conversation

ianswett
Copy link
Contributor

I'm not sure this is necessary, but I've had at least 2 people be unsure about whether a packet is still in flight if all the stream data in it is cancelled, so maybe it's worth stating this explicitly?

@@ -102,7 +102,8 @@ In-flight:

: Packets are considered in-flight when they have been sent
and neither acknowledged nor declared lost, and they are not
ACK-only.
ACK-only. Packets are still considered in-flight even if
their payloads no longer need to be delivered.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
their payloads no longer need to be delivered.
their payloads no longer need to be delivered in order to maintain the congestion window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about what you're trying to add here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I got the in-flight concept wrong, but in order to make the definition significant, there must be a reason for it, otherwise it is just semantics. I.e. why does it matter whether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In-flight packets count towards bytes in flight, which is defined in the pseudocode section, but we could move that definition up here if it's helpful.

@ianswett ianswett added editorial An issue that does not affect the design of the protocol; does not require consensus. -recovery labels Dec 17, 2018
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this is necessary or even helpful. If we accept that information is what is repaired, rather than packets or frames, then we don't need to concern ourselves with this clarification. To that end, packets are disposable, just as frames are. What matters here is that packets are neither lost nor acknowledged, which the existing text covers nicely.

@ianswett
Copy link
Contributor Author

I updated this to be a clarifying example, which I think is more helpful than re-iterating the previous sentence. PTAL.

@@ -102,7 +102,9 @@ In-flight:

: Packets are considered in-flight when they have been sent
and neither acknowledged nor declared lost, and they are not
ACK-only.
ACK-only. For example, packets containing STREAM frames are
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the "for example" is missing context. Perhaps:

: Packets are considered in-flight when they have been sent and neither acknowledged nor declared lost. ACK-only packets are not considered in-flight, though all other packets are, including packets that contain data that would not otherwise be sent again, such as a packet containing only STREAM frames for a reset stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me. @janaiyengar what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After starting to add this in, I don't think it reads as clearly as I'd like, so I didn't. However, I did fix an error in the pseudocode below involving ACK-only packets not being considered in-flight.

@ianswett
Copy link
Contributor Author

I fixed the pseudo-code issue in #2219 so I'm going to close this, since I think it's doing more harm than good.

@ianswett ianswett closed this Dec 21, 2018
@MikeBishop MikeBishop deleted the ianswett-in-flight branch February 6, 2019 00:11
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants