HTTP2: HttpClient does not respond to pings after GOAWAY when some streams are still active

[krwq]

After client receives GOAWAY it should still respond to PING as long as there are any streams open.

Details:

• GOAWAY frame contains two parameters: streamId => always 0, lastStreamId > last stream server can take action on (this means the streams with lower or equal number than that should still continue)
• GOAWAY tells that connection is that server no longer wishes for new streams to be open on the same connection and that client should not respond to frames with higher id than lastStreamId
• PING per rfc should be treated with high priority and should be responded to

This behavior seem to not be explicitly defined in the spec (at least I could not find any mentions) but:

• spec says that communication on streams with lower stream ids than last stream id should continue (GOAWAY and PING are both not tied to specific stream id but ID they send frame to is lower than lastStreamId)
• after GOAWAY is send it is particularly important to respond to pings as it seems it may be used by server to determine if client immediately went away or not
• we currently respond to still active streams but not on streamless frames like PING
• not responding to PING means that active stream might be unnecesarily closed and will need to be retried on the new connection

[krwq]

cc: @geoffkizer @wfurt @stephentoub, I've marked this as 3.0 because this feels like something we should address (active streams might get unnecessarily closed if client doesn't respond to pings), please let me know if you think otherwise. Other reason is that it also helps with validating GOAWAY scenarios: PINGs can be used to validate that we have received GOAWAY (possibly also other frames which do not require ack if there are such)

[wfurt]

I don't think it is legal to send ping after GOAWAY

5.4.1.  Connection Error Handling

 The GOAWAY frame  includes an error code that indicates why the connection is  terminating.  After sending the GOAWAY frame for an error condition,   the endpoint MUST close the TCP connection.

With that I see no way how endpoint can send ping afterward.

[krwq]

@wfurt - after GOAWAY every stream with id less or equal than GOAWAY's lastStreamId is still valid and until those stream are shutdown the connection is still on (the server won't accept new streams though).

If client decides to not respond to the PING the server is allowed to terminate the connection: server has previously said it will still continue parsing some of the streams so now this declaration would go to waste. I think this might be a spec issue (or not specified enough).

[wfurt]

Seems like section 6.8 describes more than 5.4.1.
I''m wonder what Kestrel does.
cc: @JamesNK @Tratcher

[Tratcher]

@wfurt when Kestrel receives a GoAway it continues to process all incoming frames until all the active streams are complete. The client should do the same. Note even in the error case it's the sender that decides if it wants to cease normal operations and close the connection.

[wfurt]

I look again and ProcessGoAwayFrame() calls AbortStreams() with last valid stream and tht calls CheckForShutdown() where

            // Check if dictionary has become empty
            if (_httpStreams.Count != 0)
            {
                return;
            }

so we should not hit the endgame if we have pending streams. We still process incoming frames so I see no obvious reason why ping would not work.

[krwq]

@wfurt the problem is specifically with https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L1186

This is causing us to throw for PING but not for other streams - for open streams we seem to be bypassing the _disposed check.

Currently Abort (user code) and GOAWAY seem to be treated the same, I believe the correct solution would be to set different flag (i.e. _terminating = true) on GOAWAY and _disposed should be set when the last stream is shutdown or Abort

[wfurt]

maybe first step is to validate that existing streams can successfully finish I think distinguishing cases when somebody called Disposed() from receiving GOAWAY can be useful.

[krwq]

@wfurt I've already tested this as part of dotnet/corefx#38561 and works correctly for simple scenarios but there are conditions (like this issue) which cause failures with that.

I believe there are more frames which need similar fix (possibly SETTINGS frame and others as well) so I think this issue should cover tiny cleanup on Dispose vs GOAWAY

[karelz]

Does not seem to be critical for gRPC, moving to Future.

[geoffkizer]

Currently Abort (user code) and GOAWAY seem to be treated the same, I believe the correct solution would be to set different flag (i.e. _terminating = true) on GOAWAY and _disposed should be set when the last stream is shutdown or Abort

Yeah, there is some weirdness in how we track shutdown and why the connection is shutting down. I agree that it would be good to distinguish these cases better.

[krwq]

I'm marking this as 3.0 since otherwise there is no good way to write tests for some other 3.0 GOAWAY related issues and also this will get fixed in dotnet/corefx#39036 (this was not targeting this specific issue but fix is related).