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

change the STREAM_ID_BLOCKED frame to encode stream 0 #1851

Closed
wants to merge 2 commits into from

Conversation

marten-seemann
Copy link
Contributor

Fixes #1850.

: A variable-length integer indicating the highest stream ID that the sender
was permitted to open.
: A variable-length integer indicating the stream ID of the stream that the
sender was not able to open due to the maximum stream ID limit.
Copy link
Member

@kazuho kazuho Oct 11, 2018

Choose a reason for hiding this comment

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

To avoid ambiguity, would it make sense to clarify that the value is "the maximum stream ID among the streams that the sender is willing to open"?

Consider the case of a client that received from server a initial_max_bidi_stream of zero. If the client wants to open two bidirectional streams (i.e. streams with ID 0 and 4), what is the stream ID to be encoded in the STREAM_ID_BLOCKED frame? My intent is to clarify that sending 4 in allowed such case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PHAL.

@marten-seemann
Copy link
Contributor Author

I’d argue that sending 4 in that case is not correct. Streams are opened sequentially, so you’re blocked on opening stream 0, not on opening stream 4.

@MikeBishop
Copy link
Contributor

The original reason for adding the Stream ID was to deduplicate and tell whether a frame was delayed or if the peer is still blocked. Including that you'd like to open "up to" Stream 4 is actually more informative, since it tells the recipient that merely allowing a single additional stream doesn't unblock you.

@marten-seemann
Copy link
Contributor Author

You’re right. This is how it should work. This goes beyond the intent of this PR, which was supposed to just fix an encoding issue. I’ll update it.

@martinthomson
Copy link
Member

Including that you'd like to open "up to" Stream 4 is actually more informative

Yes, but that creates a new manner in which this frame differs from the other frames. If we make this change, then we need to be clearer about the semantics of this and the other *_BLOCKED frames. Looking now, I'm not clear on whether STREAM_BLOCKED sends the current window, or the maximum offset that the sender wanted to send.

@marten-seemann
Copy link
Contributor Author

@martinthomson: Would it help if I deleted the last commit to move this PR forward?

@martinthomson
Copy link
Member

A variable-length integer indicating the maximum stream ID when the sender was unable to open a stream.

That makes it closer to the effective definition of the other limits. Then we need to modify the others to match.

@@ -3254,8 +3254,8 @@ The STREAM_ID_BLOCKED frame contains a single field.

Stream ID:

: A variable-length integer indicating the highest stream ID that the sender
was permitted to open.
: A variable-length integer indicating the maximum stream ID of the streams that
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would remove "of the streams that" because I think it makes this sentence less clear.

martinthomson added a commit that referenced this pull request Oct 19, 2018
Recent discussion around the use of BLOCKING frames highlighted some
inconsistency - and even disagreement - about what the purpose of the
frames are.  If you read through all the text for the flow
control-related frames, it is clear that the frame carries the *limit*
at the time the frame is sent.

> These frames always include the limit that is causing blocking at the
> time that they are transmitted.

This enacts that change more consistently, especially for
STREAM_ID_BLOCKED, which was the most unclear.

The discussion on #1851 suggested that there was value in knowing what
the sender (or stream opener) wanted to get to.  That is, the frames
would carry a higher value.  If the limit was X and the sender wanted to
send octet X+Y (or open stream X+Y), they would convey that information
instead.  The theory here is that the larger value (X+Y) would be sent,
allowing the receiver to make those resources available.

The problem with this alternative approach is that the value advertised
changes over time and it is difficult to connect the signal (a BLOCKED
frame), with the limit that was in force at the time.

I suspect that there is value in signaling the desired limits as well,
but that would require greater justification.  It also entails a change
and I'm leery of feature creep at this stage.

Closes #1851, #1850.
martinthomson added a commit that referenced this pull request Oct 26, 2018
@janaiyengar
Copy link
Contributor

OBE. See #1906.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants