Subject: [quicwg/base-drafts] allow STOP_SENDING on implicitly opened streams (#2352)

From: Marten Seemann <[email-masked]>
Date: Fri, 18 Jan 2019 20:13:25 -0800

As suggested in https://github.com/quicwg/base-drafts/issues/2345#issuecomment-455448227.
Fixes #2345.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/2352

-- Commit Summary --

  * allow STOP_SENDING on implicitly opened streams

-- File Changes --

    M draft-ietf-quic-transport.md (7)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2352.patch
https://github.com/quicwg/base-drafts/pull/2352.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352

---

From: MikkelFJ <[email-masked]>
Date: Fri, 18 Jan 2019 22:06:32 -0800

I am not necessarily objecting to this, but before just merging this in, please review past discussions on this - it is not as trivial as it might seem.

https://github.com/quicwg/base-drafts/issues/1050
https://github.com/quicwg/base-drafts/issues/1797

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455753063

---

From: Marten Seemann <[email-masked]>
Date: Fri, 18 Jan 2019 22:22:54 -0800

What concrete problem do you see, @mikkelfj?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455753870

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 01:06:27 -0800

> What concrete problem do you see, @mikkelfj?

I don't see a concrete problem, but I do remember a lot of pitfalls, so I just wanted to make a note of it before anything gets merged. I have not yet read through the issues in detail.

One concern is race conditions but it might be captured by the proposed text, I'm just not sure yet.

Also, it is difficult to grasp exactly what is the difference between not yet created, created, Ready, peer has seen data from higher stream but ID is below MAX_STREAM_ID.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455762660

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 01:53:40 -0800

propose:

Receipt of a STOP_SENDING frame is invalid for a locally-initiated stream that
has not yet been created or is in the "Ready" state (see
{{stream-send-states}}). Receiving a STOP_SENDING frame for a locally-initiated
stream that has not transitioned past Ready state MUST be treated as a connection error of type
STREAM_STATE_ERROR unless there is a higher numbered stream that has made this transition already.  A peer must have seen data on the same stream or a higher numbered stream of same type before sending STOP_SENDING. An endpoint that receives a STOP_SENDING frame for a
receive-only stream MUST terminate the connection with error STREAM_STATE_ERROR.

See also discussion on associated issue. Removing Ready is not safe as this can lead to speculative STOP_SENDING, for example based on assumed out of band knowledge of the senders local state.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455765790

---

From: Kazuho Oku <[email-masked]>
Date: Sat, 19 Jan 2019 02:12:54 -0800

kazuho commented on this pull request.

> @@ -4491,10 +4491,9 @@ to abruptly terminate transmission on a stream.
 Receipt of a STOP_SENDING frame is invalid for a locally-initiated stream that
 has not yet been created or is in the "Ready" state (see
 {{stream-send-states}}). Receiving a STOP_SENDING frame for a locally-initiated
-stream that is "Ready" or not yet created MUST be treated as a connection error
-of type STREAM_STATE_ERROR.  An endpoint that receives a STOP_SENDING frame for
-a receive-only stream MUST terminate the connection with error
-STREAM_STATE_ERROR.
+stream that is not yet created MUST be treated as a connection error of type

I think the other approach we might consider is to state that a stream enters a Ready state when the endpoints send a frame containing a higher stream ID.

Taking that approach might help resolve @mikkelfj's concern that the definition is becoming loose.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#pullrequestreview-194351985

---

From: Marten Seemann <[email-masked]>
Date: Sat, 19 Jan 2019 02:13:46 -0800

I don't think we gain anything by worrying about out-of-band knowledge about stream states, and I don't think that @mikkelfj's text suggestion is readable. The same would apply to MAX_STREAM_DATA frames for remote-opened streams as well.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455767160

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 02:15:41 -0800

Note that just edited my text. I think @kazuho has a point. But it still needs to be clear what the remote endpoint must observe before sending.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455767291

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 02:18:54 -0800

> The same would apply to MAX_STREAM_DATA 

I think that needs to be fixed as well. The frame might get lost due to race conditions.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455767499

---

From: Marten Seemann <[email-masked]>
Date: Sat, 19 Jan 2019 02:20:24 -0800

> I think that needs to be fixed as well. The frame might get lost due to race conditions.

Can you elaborate where you see a race condition here?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455767591

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 02:24:25 -0800

> Can you elaborate where you see a race condition here?

The peer sends MAX_STREAM_DATA on stream x and stream x + 4 because they are "always" opened together. The sending endpoint has only set stream x in transmission mode and is about to do the same for stream x + 4 when it receives MAX_STREAM_DATA. Since MAX_STREAM_DATA is sent to a non-existent stream it either faithfully terminates the connection, or just ignores the frame since it is not between min/max range of active streams.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455767838

---

From: Marten Seemann <[email-masked]>
Date: Sat, 19 Jan 2019 02:31:04 -0800

I don't see any race condition we need to fix here. If it was permissible to send MAX_STREAM_DATA x+4, no amount of reordering will make this a protocol violation. If it was not permissible, and the remote peer just happened to open x+4 while the MAX_STREAM_DATA frame was in flight, then the peer got lucky this one time and the protocol violation won't be detected. I don't think this is something we need to address though.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455768292

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 02:33:50 -0800

>  If it was permissible to send MAX_STREAM_DATA x+4, no amount of reordering will make this a protocol violation

then you have to be able track infinite amount state because the peer controls MAX_STREAM_ID.

> If it was not permissible, and the remote peer just happened to open x+4 while the MAX_STREAM_DATA frame was in flight, then the peer got lucky this one time and the protocol violation won't be detected.

You can always get lucky, by it is sufficient to risk enforcement, and more importantly, it protects against tracking infinite state.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455768522

---

From: Marten Seemann <[email-masked]>
Date: Sat, 19 Jan 2019 02:36:29 -0800

> then you have to be able track infinite amount state because the peer controls MAX_STREAM_ID.

No, you don't, because you're allowed to silently drop MAX_STREAM_ID frames for closed streams. We have text about that already.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455768679

---

From: MikkelFJ <[email-masked]>
Date: Sat, 19 Jan 2019 02:55:36 -0800

> No, you don't, because you're allowed to silently drop MAX_STREAM_ID frames for closed streams. We have text about that already.

I am talking about not yet opened streams.
But is you said in the #2353 there is text protecting against this - only I believe that the text is not very precise.
Wrt. STOP_SENDING I basically want the same protection, and for both frames, I'd like it to be clear what the sender of these reverse frames need to observe before concluding that it is safe to send them.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455769914

---

From: Benjamin Saunders <[email-masked]>
Date: Sat, 19 Jan 2019 10:15:30 -0800

Ralith commented on this pull request.

Enters the Send state, I think you mean? Ready is the state that currently forbids STOP_SENDING.

That seems like an effective solution, although I find this use of "created" intuitive and consistent with the "Within each type, streams are created with numerically increasing stream IDs" language.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#discussion_r249252818

---

From: Benjamin Saunders <[email-masked]>
Date: Sat, 19 Jan 2019 10:20:29 -0800

I agree that MAX_STREAM_DATA and STOP_SENDING should have exactly the same rules for when they're accepted, and that MAX_STREAM_DATA's current language, while somewhat vague, permits the case that this issue enables for STOP_SENDING. When we settle on a way to phrase this, it might be nice to use exactly the same language for both frames to make that clear.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-455803238

---

From: Martin Thomson <[email-masked]>
Date: Sun, 20 Jan 2019 19:25:08 -0800

martinthomson commented on this pull request.

I think that we can do a little more (for instance, to the preceding sentence).  See #2354.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#pullrequestreview-194434840

---

From: Martin Thomson <[email-masked]>
Date: Tue, 22 Jan 2019 15:43:06 -0800

Closed #2352.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#event-2090275634

---

From: Martin Thomson <[email-masked]>
Date: Tue, 22 Jan 2019 15:43:05 -0800

#2354 supercedes this.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2352#issuecomment-456609020