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

STOP_SENDING in Ready state #1797

Closed
kazuho opened this issue Sep 25, 2018 · 29 comments · Fixed by #1802
Closed

STOP_SENDING in Ready state #1797

kazuho opened this issue Sep 25, 2018 · 29 comments · Fixed by #1802
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@kazuho
Copy link
Member

kazuho commented Sep 25, 2018

Section 7.15 of the current editor's draft states:

Receiving a STOP_SENDING frame for a send stream that is “Ready” or non-existent MUST be treated as a connection error of type PROTOCOL_VIOLATION.

To put it another way, if an endpoint wants to abruptly discard a bidirectional stream that it has opened, it needs to either:

  • wait for the peer to send a STREAM frame before sending STOP_SENDING
  • bundle an empty STREAM frame (offset=0, len=0, FIN not set) when sending a STOP_SENDING frame

However, either of the two solution seems like an unnecessary complexity.

Can't we simply state that a STOP_SENDING frame implicitly creates a send stream before it is applied?

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Sep 25, 2018
@martinthomson
Copy link
Member

Seems reasonable. Do you think that you could make a PR for this? I assume that you want to limit this to bidirectional streams. We don't want STOP_SENDING to be used as a means of using unidirectional streams.

@kazuho
Copy link
Member Author

kazuho commented Sep 25, 2018

Yes. This should be limited to bidirectional streams. I'll try to come up with a PR.

@MikeBishop
Copy link
Contributor

This seems like an oversight, so I'm in favor of correcting in. However, I'm not entirely convinced it's unreasonable to send STOP_SENDING preemptively for unidirectional streams either.

If you had advance knowledge of what was going to be on that stream (think HTTP/2-style PUSH_PROMISE), it might be reasonable to send STOP_SENDING for a stream that hasn't actually had data sent yet. You can presume the data is in flight based on other application state, for example.

This might also be used to "claw back" unidirectional stream ID space -- you sent MAX_STREAM_ID=4X, then decide to shrink it by sending a STOP_SENDING on X-N streams, though that might not be a pattern we want to encourage.

@MikeBishop
Copy link
Contributor

...thus a simpler fix might be to remove the prohibition, and simply clarify that the delayed allocation of Stream ID only applies to locally-initiated streams.

@kazuho
Copy link
Member Author

kazuho commented Sep 27, 2018

@MikeBishop Clarification question: you seem to argue that it is not "unreasonable to send STOP_SENDING preemptively for unidirectional streams", and that we should "clarify that the delayed allocation of Stream ID only applies to locally-initiated streams".

However, my understanding is that if sending STOP_SENDING for unidirectional streams can only be done for remotely-initiated streams, by endpoints that have prior knowledge of what the stream ID of those streams would be.

So to me it seems that we can only have either one of the properties; i.e. either allow sending STOP_SENDING preemptively, or disallow delayed allocation of stream IDs.

Regarding the general idea of having a prior knowledge of what the stream IDs will be used for, I am not sure if we want to allow (or encourage) that type of design. IIUC we discussed that and the conclusion was to introduce the "type" octet for the unidirectional streams in the HTTP binding. I prefer sticking to the decision we made.

@mikkelfj
Copy link
Contributor

mikkelfj commented Sep 27, 2018

We cannot have allow stream state modifying messages from peer on streams that have not yet been opened by the initiator, or which have been closed and possibly forgotten. The exception is that if a higher stream is opened than a previously openened, the prevoiusly unopened stream is implicitly opened.

If STOP_SENDING could happen on a future stream then this it could happen on much higher valued streams and this cannot be managed other than by ignoring or producing an error - where an error is the better option.

If STOP_SENDING is intended for an implicitly opened stream, there shouldn't be any problems, so if this "based on prior knowledge" is the fact that a higher stream was opened, then I guess it is fine.

@mikkelfj
Copy link
Contributor

After some more though - if STOP_SENDING is allowed on any stream that has not been closed and which is within a previously transmitted MAX_STREAM_ID value, I guess it could reasonably work, but you risk our of order conflicts on that.

@RyanTheOptimist
Copy link
Contributor

On the list, I raised the following question which seems quite related to this issue:

The section on STOP_SENDING says:

Receipt of a STOP_SENDING frame is only valid for a send stream that
exists and is not in the "Ready" state (see Section 3.1). Receiving
a STOP_SENDING frame for a send stream that is "Ready" or non-
existent MUST be treated as a connection error of type
PROTOCOL_VIOLATION.

However, I'm not sure I'm clear on what should be done in the face of reordering, but maybe this is obvious. Say an HTTP client sends a request. This sends a STREAM frame to the server. This move the stream to the "Send" state. Then, the user cancels the request. So the client sends a RST_STREAM and a STOP_SENDING in order to completely nuke the stream. However, both the RST_STREAM and STREAM frames are lost/reordered such that the STOP_SENDING is the first frame to arrive at the server. As such, the stream is non-existent I think, which is prohibited. But maybe I'm not thinking about this the right way?

From @MikeBishop "Ah, I see what you’re saying – the STREAM frame in the C2S direction is what moves the S2C direction from Ready to Send. If STOP_SENDING can be reordered relative to that frame, this could cause a valid-when-sent STOP_SENDING frame to arrive while the S2C direction is still in the Ready state.

I think you’re correct; the prohibition on receiving STOP_SENDING in the Ready state is erroneous. Please file an issue, and thank you!"

@mikkelfj
Copy link
Contributor

Also note the related problem:

If a stream is opened, lower valued streams are also opened. It is not possible to send STOP_SENDING to those lower valued streams because they are in ready state and have not received any data yet. It would cause a protocol violation.

@mikkelfj
Copy link
Contributor

@martinthomson I think this issue needs to be reopened based on the above comments.

@RyanTheOptimist
Copy link
Contributor

(]@mikkelfj I thought we dropped the language about streams being opened implicitly by virtue of receive information for a higher numbered stream? My recollection is that this was no longer needed which we switched from limiting the number of open streams to using max stream id.

@mikkelfj
Copy link
Contributor

@RyanatGoogle that is true, but a lot of people wanted it back, so it reentered. My primary concern was the ability to have inifinite number of streams open - either very old are very future - but there is now a limited set of ID's you need to track. For this to work, it is very important that messages on old or uncreated streams are hard errors. This is the motiviation for the STOP_SENDING in Ready being a hard error. But there are some edge cases as you discovered.

@MikeBishop MikeBishop reopened this Nov 15, 2018
@MikeBishop
Copy link
Contributor

STOP_SENDING on a stream that's already closed is a no-op, so they can be ignored. @mikkelfj, would it satisfy your concerns if STOP_SENDING on a stream above the MAX_STREAM_ID were an error?

@mikkelfj
Copy link
Contributor

@MikeBishop No I don't think it is quite right, although also not catastrophic since impact is bounded.

I think the general idea is that the non-initiator cannot respond before seeing physical evidence of a stream. To honor that principle, STOP_SENDING should only be sent for streams with an ID at below a stream that has left Ready state. I can't really wrap my head fully around this atm. because there are different kinds of streams.

If you allowed up to MAX_STREAM_ID you could open a stream that the initiator is not prepared for yet. Which is why sending STOP_SENDING is a violation in the first place.

Perhaps the solution is to have a stream state earlier than Ready which is not pemitted to receive anything unprovoked, or perhaps Ready is that state, but then its semantics should be slightly redefined?

@MikeBishop
Copy link
Contributor

The specific race condition that @RyanatGoogle raised is a STOP_SENDING on a bidirectional stream by the initiator of that stream. I think a constraint on non-initiators is probably reasonable -- there's some risk that the initiator will have "leaked" knowledge of that stream in a different context, but I would expect that will typically reflect that the initiator has moved beyond the Ready state.

@MikeBishop
Copy link
Contributor

The difficulty is that, in this specific race condition, STOP_SENDING is the first frame received from the initiator. Perhaps the right solution is that there's a different start state for peer-initiated streams than for locally-initiated streams? Locally-initiated streams begin as "Not Allocated" and become "Ready" when the peer locally decides to allocate them; receiving any frame is an error. Peer-initiated streams become "Ready" upon receipt of any frame in either direction.

@mikkelfj
Copy link
Contributor

I think it should be permissable to receive STOP_SENDING on a Ready stream because a stream is only ready if a higher stream has been created and started sending (I think). When the receiver sees that transmission it would know that earlier streams are potentially able to transmit (and addition to being ready to send). If the initiator does not want to waste state on tracking STOP_SENDING, it should not transmit on a higher ID first.

Alternatively, we should just accept this limitation - that you can't stop a transmission before seeing evidence of it being active. All the implicit open logic is about being able to transmit while STOP_SENDING affects the peers send state - which is unfortunate. It's really bordering cause/effect in particle physics (not entirely coincidental).

@mikkelfj
Copy link
Contributor

btw: not arguing against your proposal on "Not Allocated" I think it is already there somewhat implicit. That might be the way to go - the bigger question is - what is the desired behavior in the first place.

@MikeBishop
Copy link
Contributor

I would say the overall desired behavior is that the initiator of a stream must be the first one to send frames on it, but that any frame it chooses to send should be considered valid, regardless of reordering.

@MikeBishop
Copy link
Contributor

The other way to frame it is that a peer-initiated stream moves out of Ready on receipt of any frame, and the prohibition on STOP_SENDING gets scoped to locally-initiated streams.

@RyanTheOptimist
Copy link
Contributor

"I would say the overall desired behavior is that the initiator of a stream must be the first one to send frames on it, but that any frame it chooses to send should be considered valid, regardless of reordering."

"The other way to frame it is that a peer-initiated stream moves out of Ready on receipt of any frame, and the prohibition on STOP_SENDING gets scoped to locally-initiated streams."

@MikeBishop well said. I completely agree with both framings of this issue.

@mikkelfj
Copy link
Contributor

I think the answer to @RyanatGoogle 's problem is that it can't really send a STOP_SENDING together with a RESET_STREAM unless a higher stream was already open. Not because of the current formulation, but because it violates the principle of sender creating the stream state for its own half-stream, regardless of who is the initiater.

To work around that, either STOP_SENDING should be permitted up to MAX_STREAM_ID, which I dislike, or a new STOP_RESET_STREAM message is needed, or STOP_SENDING is only permitted when coalesced in the same packet, which is messy.

So it really is the same problem as the uninitated peer problem - STOP_SENDING is invasive on streams that are not yet ready.

@RyanTheOptimist
Copy link
Contributor

"but because it violates the principle of sender creating the stream state for its own half-stream, regardless of who is the initiater."

Is that correct? I think the stream state machine disagrees:

          o
          | Create Stream (Sending)
          | Create Bidirectional Stream (Receiving)
          v
      +-------+

This says that the send side of a bidi stream is opened when a bidi stream is opened on the receive side. Or perhaps I'm misreading that?

@mikkelfj
Copy link
Contributor

I'm note sure, I have a hard time reading these diagrams. You need to also read the associated text at the very least.

Even with a STOP_RESET_STREAM that closed both half-streams, you still have the problem that you then potentially opened streams with lesser ID's. You cannot send STOP_SENDING on these due to the same race condition.

@mikkelfj
Copy link
Contributor

This says that the send side of a bidi stream is opened when a bidi stream is opened on the receive side. Or perhaps I'm misreading that?

I don't think that is wrong, but you cannot know the state at both ends at the same time. So the question is how you know that the other end opened the stream. It can happen by receiving data, or via implicit open in some cases.

I start to dislike implicit open even more now.

@MikeBishop
Copy link
Contributor

Looking at the existing transitions....

The sending part of a bidirectional stream initiated by a peer (type 0 for a server, type 1 for a client) enters the “Ready” state then immediately transitions to the “Send” state if the receiving part enters the “Recv” state (Section 3.2).

And in "Send," STOP_SENDING is valid. So what causes the receiving part to enter "Recv"?

The receiving part of a stream initiated by a peer (types 1 and 3 for a client, or 0 and 2 for a server) is created when the first STREAM, STREAM_DATA_BLOCKED, RESET_STREAM, or MAX_STREAM_DATA (bidirectional only, see below) is received for that stream. The initial state for a receive stream is “Recv”.

It's odd to say that STOP_SENDING causes a state change on the receive stream, but that's one way to formulate this. It's evidence that the peer is starting to use this bidirectional stream, and is no more weird than a MAX_STREAM_DATA (which is similarly talking about the other stream direction).

Maybe STOP_SENDING needs to join the bidirectional-only category of frames that can open a stream in both directions.

@mikkelfj
Copy link
Contributor

Maybe STOP_SENDING needs to join the bidirectional-only category of frames that can open a stream in both directions.

And maybe there or more hidden race conditions in other frame types?

@ianswett
Copy link
Contributor

I do think STOP_SENDING needs to join the set of frames that open a stream in both directions.

In particular, if I sent a GET in HTTP, and then I send a STOP_SENDING and RESET_STREAM in the same packet a few ms later, but the packet containing the GET is lost, I think that needs to be valid, since I expect webapps to do that fairly frequently.

@MikeBishop
Copy link
Contributor

And maybe there or more hidden race conditions in other frame types?

From a cursory glance over the frame types, STOP_SENDING and MAX_STREAM_DATA appear to be the two frames sent by receivers on a stream. Everything else is either connection-level or sent by senders. The sender frames seem to be well-covered in terms of opening a stream.

@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants