-
Notifications
You must be signed in to change notification settings - Fork 565
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
Identifying streams more precisely #669
Conversation
@@ -2068,8 +2068,9 @@ HTTP2-Settings = token68 | |||
control. The initial value is 2<x:sup>16</x:sup>-1 (65,535) octets. | |||
</t> | |||
<t> | |||
This setting affects the window size of all streams, including existing streams, | |||
see <xref target="InitialWindowSize"/>. | |||
This setting affects the window size of all streams in the "open" or "half-closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've changed the meaning here. Modifying this affects (original) "all streams, including existing streams" -- now it only affects existing streams (open/half-closed (remote)). I'd keep it as all streams, maybe add "regardless of state" if you feel the clarification is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as you note below, these are the only states in which the setting has any real meaning (you can't send DATA in any other state). I'd be happy to drop the mention of states though. I was only trying to be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that this not obscure the fact that it also changes the window size for all idle streams on which you might be able to send data in the future. Hopefully obvious, but let's try not to confuse an overly-literal reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yes. I will drop the specific stream states from this specific statement (less text FTW).
Identifying streams more precisely
Two issues were raised about the use of the words "existing" and "current" to refer to streams. These were a little squishy, so I tightened the language a little. The text no longer uses either word to refer to streams, instead using stream states to describe where the relevant text applies and favouring "identified" as a fallback.