Subject: [quicwg/base-drafts] Move GOAWAY from transport to HTTP (#697)

From: Mike Bishop <[email-masked]>
Date: Wed, 26 Jul 2017 12:20:09 -0700

Fixes #696.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Moved GOAWAY from transport to HTTP

-- File Changes --

    M draft-ietf-quic-http.md (115)
    M draft-ietf-quic-transport.md (76)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/697.patch
https://github.com/quicwg/base-drafts/pull/697.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/697

---

From: Mike Bishop <[email-masked]>
Date: Wed, 26 Jul 2017 12:25:09 -0700

@MikeBishop pushed 1 commit.

c08f315  Fix bad wrapping

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/697/files/f0dcde1eb64aaa5105e390e4227a74fcb18bfc3c..c08f31509e9d36c5bcc1eb05c596e2542d2129b3

---

From: Martin Thomson <[email-masked]>
Date: Wed, 26 Jul 2017 18:10:38 -0700

martinthomson requested changes on this pull request.

I think that this is the right general approach, though I would want to discuss on the list first.

There is a specific part of the design that I don't think works here (or in h2).  That might be something we discuss separately though.  I'll raise that on the issue, so that we don't get side-tracked with discussion on PRs.

> +The GOAWAY frame applies to the connection, not a specific stream. An endpoint
+MUST treat a GOAWAY frame on a stream other than the control stream as a
+connection error ({{errors}}) of type HTTP_WRONG_STREAM.
+
+The last stream identifier in the GOAWAY frame contains the highest-numbered
+stream identifier for which the sender of the GOAWAY frame might have taken some
+action on or might yet take action on.  All streams up to and including the
+identified stream might have been processed in some way.  The last stream
+identifier can be set to 0 if no streams were processed.
+
+> Note: In this context, "processed" means that some data from the stream was
+> passed to some higher layer of software that might have taken some action as a
+> result.
+
+If a connection terminates without a GOAWAY frame, the last stream identifier is
+effectively the highest possible stream identifier.

MAX_STREAM_ID determines this...

> +
+The GOAWAY frame does not define any flags.
+
+The GOAWAY frame applies to the connection, not a specific stream. An endpoint
+MUST treat a GOAWAY frame on a stream other than the control stream as a
+connection error ({{errors}}) of type HTTP_WRONG_STREAM.
+
+The last stream identifier in the GOAWAY frame contains the highest-numbered
+stream identifier for which the sender of the GOAWAY frame might have taken some
+action on or might yet take action on.  All streams up to and including the
+identified stream might have been processed in some way.  The last stream
+identifier can be set to 0 if no streams were processed.
+
+> Note: In this context, "processed" means that some data from the stream was
+> passed to some higher layer of software that might have taken some action as a
+> result.

style nit: use a DL for the note:

```
Note:
: In this context, ...
```

> +identified by a QUIC MAX_STREAM_ID frame.
+
+Once sent, the sender will refuse requests sent on streams initiated by the
+receiver if the stream has an identifier higher than the included last stream
+identifier.  Receivers of a GOAWAY frame MUST NOT send new requests on the
+connection, although a new connection can be established for new streams.
+
+If the receiver of the GOAWAY has sent requests on streams with a higher stream
+identifier than what is indicated in the GOAWAY frame, those streams are not or
+will not be processed.  The receiver of the GOAWAY frame can treat the requests
+as though they had never been sent at all, thereby allowing those requests to be
+retried later on a new connection.  Servers SHOULD treat requests on streams
+above this ID as stream errors of type HTTP_REQUEST_CANCELLED.
+
+Endpoints SHOULD send a GOAWAY frame when the closing of a connection is known
+in advance, even if the window is small, so that the remote peer can know

What is "the window"?  I think that the point here is that you send the GOAWAY even if there is not guarantee that there will be time to ensure that the GOAWAY is properly received.  I think that includes cases where you might be sending a CONNECTION_CLOSE in response to an error condition.

Note that sending GOAWAY suffers from HOLB, which makes this a less than ideal solution.  I have some ideas about how we might avoid that, but it's an even more dramatic shift in the protocol design than unidirectional streams.  So much so that @ianswett might have an aneurysm.

> +will not be processed.  The receiver of the GOAWAY frame can treat the requests
+as though they had never been sent at all, thereby allowing those requests to be
+retried later on a new connection.  Servers SHOULD treat requests on streams
+above this ID as stream errors of type HTTP_REQUEST_CANCELLED.
+
+Endpoints SHOULD send a GOAWAY frame when the closing of a connection is known
+in advance, even if the window is small, so that the remote peer can know
+whether a stream has been partially processed or not.  For example, if an HTTP
+client sends a POST at the same time that a server closes a QUIC connection, the
+client cannot know if the server started to process that POST request if the
+server does not send a GOAWAY frame to indicate what streams it might have acted
+on.
+
+For unexpected closures caused by error conditions, transport-level mechanisms
+SHOULD be used.  However, a GOAWAY MAY still be sent to provide additional
+detail to clients.

Rather than say "transport-level mechanisms", just mention CONNECTION_CLOSE explicitly.  

Secondly, the important "additional detail" here is the stream identifier.  I think that we should remove the error code and debug data (you will note that these were removed from the transport-layer GOAWAY).

> +The last stream identifier in the GOAWAY frame contains the highest-numbered
+stream identifier for which the sender of the GOAWAY frame might have taken some
+action on or might yet take action on.  All streams up to and including the
+identified stream might have been processed in some way.  The last stream
+identifier can be set to 0 if no streams were processed.
+
+> Note: In this context, "processed" means that some data from the stream was
+> passed to some higher layer of software that might have taken some action as a
+> result.
+
+If a connection terminates without a GOAWAY frame, the last stream identifier is
+effectively the highest possible stream identifier.
+
+Reattempting requests on streams with lower- or equal-numbered identifiers is
+not possible, with the exception of idempotent actions like HTTP GET, PUT, or
+DELETE.  Any protocol activity that used higher-numbered streams can be safely

This is probably cribbed from RFC 7450 (as is most of this), however it could be improved.  I think that you want to concentrate first on what this guarantees (requests on higher stream numbers can be retried automatically), then note that other streams cannot be automatically retried unless it is otherwise permitted (for example, a request with an idempotent method).

> +> Note: In this context, "processed" means that some data from the stream was
+> passed to some higher layer of software that might have taken some action as a
+> result.
+
+If a connection terminates without a GOAWAY frame, the last stream identifier is
+effectively the highest possible stream identifier.
+
+Reattempting requests on streams with lower- or equal-numbered identifiers is
+not possible, with the exception of idempotent actions like HTTP GET, PUT, or
+DELETE.  Any protocol activity that used higher-numbered streams can be safely
+retried using a new connection.
+
+Activity on streams numbered lower or equal to the last stream identifier might
+still complete successfully.  The sender of a GOAWAY frame might gracefully shut
+down a connection by sending a GOAWAY frame, maintaining the connection in an
+"open" state until all in-progress streams complete.

We can be more concrete about this.  Say that GOAWAY doesn't cause the connection to close, and the connection will remain open until requests on streams up to the last stream ID have completed.  After that, the connection can be closed (let's avoid making a statement about silent close vs. a signaled close at this point by remaining silent on exactly *how* it is closed).

> +still complete successfully.  The sender of a GOAWAY frame might gracefully shut
+down a connection by sending a GOAWAY frame, maintaining the connection in an
+"open" state until all in-progress streams complete.
+
+An endpoint MAY send multiple GOAWAY frames if circumstances change. For
+instance, an endpoint that sends GOAWAY without an error code during graceful
+shutdown could subsequently encounter an error condition.  The last stream
+identifier from the last GOAWAY frame received indicates which streams could
+have been acted upon.  Endpoints MUST NOT increase the value they send in the
+last stream identifier, since the peers might already have retried unprocessed
+requests on another connection.
+
+A client that is unable to retry requests loses all requests that are in flight
+when the server closes the connection.  A server that is attempting to
+gracefully shut down a connection SHOULD send an initial GOAWAY frame with the
+last stream identifier set to 2^32-1.  This signals to the client that a

This requirement is no longer correct because we have MAX_STREAM_ID to govern this.  You can send a GOAWAY with the value of MAX_STREAM_ID and be reasonably sure that all in-flight requests are covered.

> +identifier from the last GOAWAY frame received indicates which streams could
+have been acted upon.  Endpoints MUST NOT increase the value they send in the
+last stream identifier, since the peers might already have retried unprocessed
+requests on another connection.
+
+A client that is unable to retry requests loses all requests that are in flight
+when the server closes the connection.  A server that is attempting to
+gracefully shut down a connection SHOULD send an initial GOAWAY frame with the
+last stream identifier set to 2^32-1.  This signals to the client that a
+shutdown is imminent and that initiating further requests is prohibited.  After
+allowing time for any in-flight requests (at least one round-trip time), the
+server MAY send another GOAWAY frame with an updated last stream identifier.
+This ensures that a connection can be cleanly shut down without losing requests.
+
+After sending a GOAWAY frame, the sender MUST reset any existing or subsequently
+opened streams with an HTTP_REQUEST_CANCELLED error code.

I think that this paragraph is incorrect.  I think that there is a missing condition here: it only does this if it wants to allow those requests to be retried.

> +
+An endpoint MAY send multiple GOAWAY frames if circumstances change. For
+instance, an endpoint that sends GOAWAY without an error code during graceful
+shutdown could subsequently encounter an error condition.  The last stream
+identifier from the last GOAWAY frame received indicates which streams could
+have been acted upon.  Endpoints MUST NOT increase the value they send in the
+last stream identifier, since the peers might already have retried unprocessed
+requests on another connection.
+
+A client that is unable to retry requests loses all requests that are in flight
+when the server closes the connection.  A server that is attempting to
+gracefully shut down a connection SHOULD send an initial GOAWAY frame with the
+last stream identifier set to 2^32-1.  This signals to the client that a
+shutdown is imminent and that initiating further requests is prohibited.  After
+allowing time for any in-flight requests (at least one round-trip time), the
+server MAY send another GOAWAY frame with an updated last stream identifier.

There's an open question here about whether a recipient of GOAWAY can initiate any more requests.  I don't think that we were sufficient clear about that in h2; this is similarly unclear.  I think that we should allow for new requests to be initiated here; after all, we have no clear demarcation point that can be used.  I think that we might advise against increasing MAX_STREAM_ID after sending GOAWAY (at least on the server side).  But that goes back to my other comments, which you will soon see on the issue.

-- 
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/697#pullrequestreview-52534677

---

From: Mike Bishop <[email-masked]>
Date: Thu, 27 Jul 2017 16:57:42 +0000 (UTC)

MikeBishop commented on this pull request.

I'll incorporate your other comments shortly.

+

If the server knows that it's going to close a connection, it should send GOAWAY to warn the client.  Errors that warrant CONNECTION_CLOSE aren't generally known in advance, but if they are, then I agree that this should include them.

Fair enough.

I think 7540 (and therefore this text) is fairly explicit on that point:

> Receivers of a GOAWAY frame MUST NOT send new requests on the connection, although a new connection can be established for new streams.

I agree that the server can't actually enforce that precisely, however, just as it can't in h2.

-- 
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/697#pullrequestreview-52718304

---

From: Mike Bishop <[email-masked]>
Date: Thu, 27 Jul 2017 10:46:02 -0700

IIRC, that text was targeted (in H2) at the disjoint case -- you supply a horizon (the GOAWAY) and then RST the individual streams below that.  I've tried to clarify that.

-- 
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/697#discussion_r129910388

---

From: Martin Thomson <[email-masked]>
Date: Fri, 28 Jul 2017 00:16:40 +0000 (UTC)

martinthomson commented on this pull request.

This is probably something that needs to be said explicitly 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/697#discussion_r129988912

---

From: Mike Bishop <[email-masked]>
Date: Fri, 28 Jul 2017 19:14:17 +0000 (UTC)

@MikeBishop pushed 2 commits.

177db7c  GOAWAY is server-only; rearranging; Martin's feedback
d7d0bc2  Merge remote-tracking branch 'origin/master' into goaway

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/697/files/c08f31509e9d36c5bcc1eb05c596e2542d2129b3..d7d0bc2cd36a26475aed54e86957837503f6f3df

---

From: Mike Bishop <[email-masked]>
Date: Fri, 28 Jul 2017 12:19:03 -0700

56cce38  More rearranging

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/697/files/d7d0bc2cd36a26475aed54e86957837503f6f3df..56cce38e24ad6d0224c4ed7e17ef178840e9e5ce

---

From: Mike Bishop <[email-masked]>
Date: Fri, 28 Jul 2017 23:34:50 +0000 (UTC)

c7160d9  Merge remote-tracking branch 'origin/master' into goaway

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/697/files/56cce38e24ad6d0224c4ed7e17ef178840e9e5ce..c7160d9397d8f2ff2b2d815d4ddc719b5655a022

---

From: Martin Thomson <[email-masked]>
Date: Mon, 31 Jul 2017 02:45:52 +0000 (UTC)

martinthomson approved this pull request.

I wonder if now is the right time to suggest that we call this frame SHUTDOWN rather than GTFO.

> @@ -591,6 +591,83 @@ The payload consists of:
   : HPACK-compressed request headers for the promised response.

+### GOAWAY {#frame-goaway}
+
+The GOAWAY frame (type=0x7) is used to initiate graceful shutdown of a
+connection by a server.  GOAWAY allows a server to stop accepting new requests
+while still finishing processing of previously received requests.  This enables
+administrative actions, like server maintenance.  GOAWAY by itself does not
+close a connection.
+
+The GOAWAY frame does not define any flags, and the payload is a QUIC stream
+identifier.  The GOAWAY frame applies to the connection, not a specific stream.
+An endpoint MUST treat a GOAWAY frame on a stream other than the control stream
+as a connection error ({{errors}}) of type HTTP_WRONG_STREAM.
+
+There is an inherent race condition between a client sending new requests and
+the server sending a GOAWAY frame.  To deal with this case, the GOAWAY contains

We never explained the nature of that race condition in 7540, and now it's bugging me.  Should we open an issue to track this?

Maybe the right way to fix this is to change the description.  Say that this frame exists to allow client and server to agree on what requests were accepted and which were rejected.  GOAWAY identifies the last request that the client can make and the server will accept.  All requests that are sent on higher-numbered streams are implicitly cancelled.

Described that way, there isn't a need to talk about the race condition.

> +GOAWAY frame.
+
+  Note:
+  : In this context, "processed" means that some data from the stream was
+    passed to some higher layer of software that might have taken some action as
+    a result.
+
+Once sent, the server will refuse requests sent on streams with an identifier
+higher than the included last stream identifier.  Clients MUST NOT send new
+requests on the connection after receiving GOAWAY, although requests might
+already be in transit. A new connection can be established for new requests.
+
+If the client has sent requests on streams with a higher stream identifier than
+indicated in the GOAWAY frame, those requests were not and will not be
+processed.  Servers SHOULD reset new streams above this ID with the error code
+HTTP_REQUEST_CANCELLED.  Servers MAY also reset streams below the indicated ID

s/Servers SHOULD reset new streams/Endpoints SHOULD reset client-initiated streams/

It's not especially graceful, but the server might see those streams as open, and cancelling them avoids problems.  Also, this isn't about the server, because clients should also reset their requests and avoid retransmissions.  

Separately, I wonder: HTTP_REQUEST_CANCELLED from the client doesn't have the effect that we want: it doesn't guarantee that the request wasn't processed by the server; it can't.  Do we need some text on that.

> +  : In this context, "processed" means that some data from the stream was
+    passed to some higher layer of software that might have taken some action as
+    a result.
+
+Once sent, the server will refuse requests sent on streams with an identifier
+higher than the included last stream identifier.  Clients MUST NOT send new
+requests on the connection after receiving GOAWAY, although requests might
+already be in transit. A new connection can be established for new requests.
+
+If the client has sent requests on streams with a higher stream identifier than
+indicated in the GOAWAY frame, those requests were not and will not be
+processed.  Servers SHOULD reset new streams above this ID with the error code
+HTTP_REQUEST_CANCELLED.  Servers MAY also reset streams below the indicated ID
+with HTTP_REQUEST_CANCELLED if the associated requests were not processed.
+
+The client can treat these requests as though they had never been sent at all,

A client can treat requests that are cancelled by the server as though they have never been sent

If the client cancels them, then they still might be live.  (Yeah, this is in the context of a server-sent thing, but it's better to have an accurate context-free statement.)

> @@ -591,6 +591,83 @@ The payload consists of:
   : HPACK-compressed request headers for the promised response.

+### GOAWAY {#frame-goaway}
+
+The GOAWAY frame (type=0x7) is used to initiate graceful shutdown of a
+connection by a server.  GOAWAY allows a server to stop accepting new requests
+while still finishing processing of previously received requests.  This enables
+administrative actions, like server maintenance.  GOAWAY by itself does not
+close a connection.

I think that we should explicitly say that a client doesn't need to send GOAWAY: it just stops sending requests.

> +The GOAWAY frame (type=0x7) is used to initiate graceful shutdown of a
+connection by a server.  GOAWAY allows a server to stop accepting new requests
+while still finishing processing of previously received requests.  This enables
+administrative actions, like server maintenance.  GOAWAY by itself does not
+close a connection.
+
+The GOAWAY frame does not define any flags, and the payload is a QUIC stream
+identifier.  The GOAWAY frame applies to the connection, not a specific stream.
+An endpoint MUST treat a GOAWAY frame on a stream other than the control stream
+as a connection error ({{errors}}) of type HTTP_WRONG_STREAM.
+
+There is an inherent race condition between a client sending new requests and
+the server sending a GOAWAY frame.  To deal with this case, the GOAWAY contains
+the stream identifier of the last client-initiated request that was or might be
+processed in this connection.  This MAY be lower than the stream limit
+identified by a QUIC MAX_STREAM_ID frame, and MAY be zero if no requsets were

typo: requsets

> +higher than the included last stream identifier.  Clients MUST NOT send new
+requests on the connection after receiving GOAWAY, although requests might
+already be in transit. A new connection can be established for new requests.
+
+If the client has sent requests on streams with a higher stream identifier than
+indicated in the GOAWAY frame, those requests were not and will not be
+processed.  Servers SHOULD reset new streams above this ID with the error code
+HTTP_REQUEST_CANCELLED.  Servers MAY also reset streams below the indicated ID
+with HTTP_REQUEST_CANCELLED if the associated requests were not processed.
+
+The client can treat these requests as though they had never been sent at all,
+thereby allowing them to be retried later on a new connection. Automatically
+retrying other requests is not possible, unless this is otherwise permitted
+(e.g. idempotent actions like GET, PUT, or DELETE).  Requests on stream IDs less
+than or equals to the stream ID in the GOAWAY frame remain open until they are
+completed successfully, reset, or the connection terminates.

The "remain open" clause here is potentially confusing.  The problem here is that stream state can't be affected by these HTTP frames.  I think that you might remove this last sentence.

-- 
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/697#pullrequestreview-53117183

---

From: Mike Bishop <[email-masked]>
Date: Sun, 30 Jul 2017 21:51:27 -0700

I think the sentence itself is worth emphasizing rather than dropping, but I agree that it's a little awkward to say that the request "remains open," since that sounds more like the stream this text refers to in 7540.  Hopefully a more appropriate phrase for a request will occur to me when it's not half past bedtime.

-- 
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/697#discussion_r130272071