Handling in-progress streams after receiving HTTP/2 GOAWAY

[tony612]

Now Gun returns errors to all existed streams and closes the connection after receiving the GOAWAY frame in HTTP/2. I think we'd better handle streams with stream id less than max_stream_id in GOAWAY. So that we can support the graceful shutdown triggered by the server. The spec is https://http2.github.io/http2-spec/#GOAWAY

I have a very simple implementation:

--- a/src/gun_http2.erl
+++ b/src/gun_http2.erl
@@ -160,9 +160,15 @@ frame(State=#http2_state{http2_machine=HTTP2Machine0}, Frame, EvHandler, EvHandl
                                StreamID, PromisedStreamID, Headers, PseudoHeaders,
                                EvHandler, EvHandlerState);
                {ok, Frame={goaway, _StreamID, _Reason, _Data}, HTTP2Machine} ->
-                       {terminate(State#http2_state{http2_machine=HTTP2Machine},
-                               {stop, Frame, 'Server is going away.'}),
-                               EvHandlerState};
+                       case State of
+                               #http2_state{streams=[]} ->
+                                       {terminate(State#http2_state{http2_machine=HTTP2Machine},
+                                               {stop, Frame, 'Server is going away.'}),
+                                               EvHandlerState};
+                               % After receiving GOAWAY, clients should continue handling current streams.
+                               _ ->
+                                       {State#http2_state{http2_machine=HTTP2Machine}, EvHandlerState}
+                       end;

But in this way, the connection will still receive new streams.

A better way is setting the connection state to closed, but don't close the connection and don't accept new streams until all in-progress streams complete. But this will need more work in gun and even cowlib.

[essen]

No the spec is https://tools.ietf.org/html/rfc7540#section-6.8

This is related to https://github.com/ninenines/gun/issues/60 (except on the other side).

I don't think any Cowlib change is needed, just a new state that refuses to create new streams.

[essen]

Turns out this is also related to #45. I have started and done most of the work on this to complete #45. I can see that for HTTP/2 it's not yet working (basically I need to write tests) and of course I will also need to implement, test and document gun:shutdown itself for #60.

See PR https://github.com/ninenines/gun/pull/206 for the work done so far.

[essen]

Perhaps unsurprisingly, it seems that Cowboy also does not currently wait when it receives a GOAWAY frame, it immediately closes the connection. So the tests I've written for Gun are failing because streams get canceled when Cowboy terminates the connection.

[essen]

Please try with the following branches:

• Gun https://github.com/ninenines/gun/pull/206
• Cowboy https://github.com/ninenines/cowboy/pull/1399

Note that the work was done specifically for improving the grpc support in your project and was requested by a customer. I will be away for a week but would love to have feedback when I get back in order to finalize this part before September. Then we'll just have the small window improvements to add I believe. If there's anything else or if I'm forgetting anything please open a new ticket listing everything that's needed and/or linking to separate issues.

[essen]

Both branches have been merged.