Subject: Re: [quicwg/base-drafts] More detailed stream states (#894)

From: ianswett <[email-masked]>
Date: Fri, 10 Nov 2017 15:42:27 +0000 (UTC)

ianswett commented on this pull request.

Thanks, this is a great improvement to the text.  Some minor suggestions, but overall looks good.

> @@ -2675,201 +2676,289 @@ created, it MUST close the connection with error code STREAM_STATE_ERROR.
 Stream ID 0 (0x0) is reserved for the cryptographic handshake.  Stream 0 MUST
 NOT be used for application data, and is the first client-initiated stream.
 
-A QUIC endpoint MUST NOT reuse a Stream ID.  Streams MUST be created
-in sequential order.  Open streams can be used in any order.  Streams
-that are used out of order result in lower-numbered streams in the
-same direction being counted as open.
+A QUIC endpoint MUST NOT reuse a Stream ID.  Open streams can be used in any
+order.  Streams that are used out of order result in lower-numbered streams in
+the same direction being counted as open.

There is both direction and type(uni/bidi).  You use 'type' below, which seems fine here as well?

nit: "being opened." not being counted as opened.

-Once all data has been either received or discarded, a sender is no longer
-obligated to update the maximum received data for the connection.
+Sending the first STREAM or STREAM_BLOCKED frame to be sent causes a send stream

nit: remove "to be sent"

-Due to reordering, an endpoint could continue receiving frames for the stream
-even after the stream is closed for sending.  Frames received after a peer
-closes a stream SHOULD be discarded.  An endpoint MAY choose to limit the period
-over which it ignores frames and treat frames that arrive after this time as
-being in error.
+In the "Send" state, an endpoint transmits - and retransmits as necessary - data
+in STREAM frames.  The endpoint respects the flow control limits of its peer,
+accepting MAX_STREAM_DATA frames.  It generates STREAM_BLOCKED frames if it

You talk about flow control a fair amount here.  Is it worth adding a 'blocked' state to the state machine?  I'm also happy with this as is and adding it later if we think it's useful.  Just a thought.

-The "closed" state is the terminal state for a stream.  Reordering might cause
-frames to be received after closing, see {{state-hc-remote}}.
+{{fig-stream-recv-states}} shows the states for the part of a stream that
+receives data from a peer.  The states for a receive stream mirror only some the

nit: "only some of"

> +is "Recv".  Receiving a RST_STREAM frame causes the receive stream to
+immediately transition to the "Reset Recvd".
+
+The receive stream enters the "Recv" state when the sending part of a
+bidirectional stream initiated by the endpoint (type 0 for a client, type 1 for
+a server) enters the "Open" state.
+
+A bidirectional stream also opens when a MAX_STREAM_DATA frame is received.
+Receiving a MAX_STREAM_DATA frame implies that the remote peer has opened the
+stream and is providing flow control credit.  A MAX_STREAM_DATA frame might
+arrive before a STREAM or STREAM_BLOCKED frame if packets are lost or reordered.
+
+In the "Recv" state, the endpoint receives STREAM and STREAM_BLOCKED frames.
+Incoming data is buffered and reassembled into the correct order for delivery to
+the application.  As data is consumed by the application and buffer space
+becomes available, the endpoint sends MAX_STREAM_DATA frames to allow the peer

MAY send?

> +   | Read  |                   | Read  |
+   +-------+                   +-------+
+~~~
+{: #fig-stream-recv-states title="States for Receive Streams"}
+
+The receiving part of a stream initiated by a peer (types 1 and 3 for a client,
+or 0 and 2 for a server) are created when the first STREAM, STREAM_BLOCKED, or
+RST_STREAM is received for that stream.  The initial state for a receive stream
+is "Recv".  Receiving a RST_STREAM frame causes the receive stream to
+immediately transition to the "Reset Recvd".
+
+The receive stream enters the "Recv" state when the sending part of a
+bidirectional stream initiated by the endpoint (type 0 for a client, type 1 for
+a server) enters the "Open" state.
+
+A bidirectional stream also opens when a MAX_STREAM_DATA frame is received.

I would move this up a paragraph so it's next to the other frames.

-- 
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/894#pullrequestreview-75787190

---

From: erickinnear <[email-masked]>
Date: Sat, 11 Nov 2017 01:17:44 +0000 (UTC)

erickinnear commented on this pull request.

-While STOP_SENDING frames are retransmittable, an implementation MAY choose not
-to retransmit a lost STOP_SENDING frame if the stream has already been closed
-in the appropriate direction since the frame was first generated.
-See {{packetization}}.
+An endpoint is expected to send STOP_SENDING frames if packets containing the
+frame are lost.  However, once either all stream data has been received or a
+RST_STREAM frame - that is, any receive stream state other than "Recv" or "Size
+Known" - sending the frame is not necessary.

Minor nit: This sentence doesn't parse for me. 
> However, once either all stream data has been received or a RST_STREAM frame sending the frame is not necessary.

Is the idea as below?  

> An endpoint is expected to only retransmit STOP_SENDING frames for receive stream states "Recv" and "Size Known". For example, if all stream data, or a RST_STREAM frame, has been received, retransmitting STOP_SENDING frames is not necessary. 

-- 
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/894#pullrequestreview-75917348

---

From: Mike Bishop <[email-masked]>
Date: Sat, 11 Nov 2017 05:31:13 +0000 (UTC)

MikeBishop requested changes on this pull request.

I'm wondering if it's time to separate frames into those sent on/about send streams, on/about receive streams, or on the connection.  It's been there implicitly, but this change makes it even more glaring.

> -
-The following transitions are valid from this state:
-
-Sending or receiving a STREAM or RST_STREAM frame causes the identified stream
-to become "open".  The stream identifier for a new stream is selected as
-described in {{stream-id}}.  A RST_STREAM frame, or a STREAM frame with the FIN
-flag set also causes a stream to become "half-closed".
-
-An endpoint might receive MAX_STREAM_DATA or STREAM_BLOCKED frames on
-peer-initiated streams that are "idle" if there is loss or reordering of
-packets.  Receiving these frames also causes the stream to become "open".
-
-An endpoint MUST NOT send a STREAM or RST_STREAM frame for a stream ID that is
-higher than the peers advertised maximum stream ID (see
-{{frame-max-stream-id}}).
+This section describes the two types of QUIC stream in terms of the states that

"states of", I think.  Otherwise, this is hard to parse.

You'd have to have it sit to the side of Send, though, with the ability to transition from Open to Blocked (no credit to start with) and the ability to oscillate between Blocked and Send, *and* the ability to close or reset from there as well.  It feels like it would complicate the concept of the state machine much more than it would clarify.

Just say that, while in the Send state, you MUST obey flow control, sending STREAM_BLOCKED if you're stuck.

-A stream in this state can be used by the endpoint to send any frame that
-mentions a stream ID.  In this state, the endpoint MUST observe advertised
-stream and connection data limits (see {{flow-control}}).
+From any of the "Open", "Send", or "Data Sent" states, an application can signal
+that it wishes to abandon transmission of stream data.  Similarly, the endpoint
+might receive a STOP_SENDING frame from its peer.  This causes the endpoint to
+send a RST_STREAM frame, which causes the stream to enter the "Reset Sent"
+state.

Pedant alert:  The wording here doesn't make it blatantly obvious that "signal ... abandon" is done by RST_STREAM.  Maybe replace "This causes the endpoint to send..." with "In either case, the endpoint sends..."?

-A stream transitions from this state to "closed" by completing transmission of
-all data.  This includes sending all data carried in STREAM frames including
-the terminal STREAM frame that contains a FIN flag.
+An endpoint MAY send a RST_STREAM to open a send stream; this causes the send
+stream to open and immediately transition to the "Reset Sent" state.

Given that the stream was in the "Open" state before you send anything, describing the RST as opening the stream feels a little awkward.  Maybe say "...as the first frame on a send stream.  This causes the send stream to immediately transition...".

-A stream also becomes "closed" when the endpoint sends a RST_STREAM frame.
+Once a packet containing a RST_STREAM has been successfully acknowledged, the

Nit:  From the receiver's side of the ACK, there are no unsuccessful acknowledgements.

-The "closed" state is the terminal state for a stream.  Reordering might cause
-frames to be received after closing, see {{state-hc-remote}}.
+{{fig-stream-recv-states}} shows the states for the part of a stream that
+receives data from a peer.  The states for a receive stream mirror only some the
+states of the send stream at the peer.  A receive stream doesn't track states on
+the send stream that can't be observed, such as the "Open" state; instead,
+receive streams track the delivery of data to the application or application
+protocol.

...some of which cannot be observed by the sender.

-If the application resets a stream that is already in the "closed" state, a
-RST_STREAM frame MAY still be sent in order to cancel retransmissions of
-previously-sent STREAM frames.
+~~~
+       o
+       | Recv STREAM / STREAM_BLOCKED / RST_STREAM

This makes it look like two RST_STREAMs are needed to enter "RST received."

-If the application resets a stream that is already in the "closed" state, a
-RST_STREAM frame MAY still be sent in order to cancel retransmissions of
-previously-sent STREAM frames.
+~~~
+       o
+       | Recv STREAM / STREAM_BLOCKED / RST_STREAM
+       | Open Paired Stream (bidirectional)
+       | Recv MAX_STREAM_DATA
+       v

On the send side, you have separate states for "I know the stream is open" and "data is flowing."  On the receive side, you go immediately to the "Recv" state.  Is there value in splitting those?

send => resend, send another, etc.

"all stream data or a RST_STREAM has been received"

-- 
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/894#pullrequestreview-75920756

---

From: Martin Thomson <[email-masked]>
Date: Wed, 29 Nov 2017 03:44:09 +0000 (UTC)

martinthomson commented on this pull request.

Yeah, blocked is a sub-state that doesn't really add much at this level of the description.

-- 
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/894#discussion_r153687803

---

From: Martin Thomson <[email-masked]>
Date: Wed, 29 Nov 2017 03:46:07 +0000 (UTC)

Probably not, honestly - it's a MUST in a sense as well.  Best to leave the normative language about that to other parts of the spec.

-- 
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/894#discussion_r153687988

---

From: Martin Thomson <[email-masked]>
Date: Wed, 29 Nov 2017 03:58:05 +0000 (UTC)

@martinthomson pushed 1 commit.

00e4eed  Giant cluster of review feedback updates

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/894/files/375b0d54eed0204713a8a113887231b8ab6e2ce1..00e4eed73ad7df7a43f4ce97b52d14c312ca1f82

---

From: Martin Thomson <[email-masked]>
Date: Wed, 29 Nov 2017 04:00:13 +0000 (UTC)

b4a6bfb  Minor reflow

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/894/files/00e4eed73ad7df7a43f4ce97b52d14c312ca1f82..b4a6bfb46a0f9b1482d16a88795019ad1c9e405a

---

From: ianswett <[email-masked]>
Date: Thu, 30 Nov 2017 19:21:14 +0000 (UTC)

One comment/question, but looks good otherwise.

> @@ -2675,201 +2676,291 @@ created, it MUST close the connection with error code STREAM_STATE_ERROR.

This used to be Streams MUST be created in sequential order.  I think there's an open issue just for that, so I'd rather keep that as is in this PR, unless it's somehow necessary to change it here?

I don't think so?

-- 
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/894#pullrequestreview-79856322

---

From: Martin Thomson <[email-masked]>
Date: Thu, 30 Nov 2017 15:53:51 -0800

(Ugh, power cut yesterday dropped my comments...)

I know.  But I don't like having two entry points to the state machine, that's worse and the prose is clear enough.  Note also here that STREAM+FIN has the exact same effect.

I considered that, and even drew it up, but it makes the states harder, not easier to describe.  The Open state is convenient as a way to discuss some of the state transitions when sending, but I couldn't find an analogous benefit on the receive side and only complications to the state machine.  The only advantage would be in describing the receive side of a bidirectional stream, and that didn't really need an extra state.

Also, it would make the two RST_STREAM frames problem a three RST_STREAM frames problem.

+   +-------+                   +-------+
+~~~

I just added it to the list and added a parenthetical, which is ugly, but breaks the loop of dependencies.

I forgot that I did that.  Since it's unenforceable, can't we just concede that open-in-order isn't strictly an interoperability requirement?  I could restore text with weasel words, but I've been convinced that strictly legislating here isn't constructive.

-- 
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/894#pullrequestreview-79730796