RFR: 8301255: Http2Connection may send too many GOAWAY frames

[Daniel Fuchs]

On Mon, 30 Jan 2023 09:09:50 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> The following behavior was observed in a test log:
>> 
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Shutting down h2c (closed=true): java.io.EOFException: HTTP/2 client stopped
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Close all streams
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Close all streams
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) sending frame: GOAWAY: length=25, streamid=0, flags=0 Error: Not an error Debugdata: Requested by user
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] AsyncSSLConnection(SSLTube(SocketTube(2))) added 34 bytes to the write queue
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] AsyncSSLConnection(SSLTube(SocketTube(2))) signalling the publisher of the write queue
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Shutting down h2c (closed=true): java.io.EOFException: HTTP/2 client stopped
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Close all streams
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Close all streams
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) sending frame: GOAWAY: length=25, streamid=0, flags=0 Error: Not an error Debugdata: Requested by user
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] AsyncSSLConnection(SSLTube(SocketTube(2))) added 34 bytes to the write queue
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] AsyncSSLConnection(SSLTube(SocketTube(2))) signalling the publisher of the write queue
>> DEBUG: [HttpClient-2-SelectorManager] [2s 716ms] Http2Connection(SSLTube(SocketTube(2))) Shutting down h2c (closed=true): java.io.EOFException: HTTP/2 client stopped 
>> etc...
>> 
>> What happens here is that attempting to send a GOAWAY frame after some error was detected might cause an error which triggers an attempt to shutdown the connection again and cause a new GOAWAY frame to be sent. This seems to happen when trying to shutdown the client and close HTTP/2 TLS connections gracefully. 
>> 
>> The fix changes the simple 'closed' boolean into a bit mask that can combined the several closed states of the connection: `Http2Connection::shutdown` has  been called, SHUTDOWN is requested (previously was closed=true), a GOAWAY frame has been sent (the connection is half closed local), a GOAWAY connection has been received (the connection is half closed remote). 
>> 
>> If the connection is already half-closed locally - a GOAWAY frame has been sent already and there's no need to send a new one.
>> 
>> This will also better help diagnose the actual state of the connection at the time `Http2Connection::shutdown` is called.
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 674:
> 
>> 672:                 // TODO: set last stream. For now zero ok.
>> 673:                 sendFrame(f);
>> 674:             }
> 
> Unrelated to this PR, even before these changes, looking at this implementation of `close()` it's odd that this method isn't setting `closed` flag to `true` and is neither calling `shutdown` to do the additional things that the `shutdown` does. I haven't yet paid a closer attention to the callers of this method to be sure if that's OK. I'm guessing it is OK, otherwise I think we would have seen issues by now.
> Maybe this `close()` method should be renamed to `sendGoAway` if that's what it is meant for?

close() is supposed to be a graceful shutdown - sending GOAWAY with no error, and that's why it doesn't close the connection immediately. You are right however that the naming sounds a bit strange. I should probably log another issue to revisit how GOAWAY is handled by the HttpClient in the HTTP/2 stack.

-------------

PR: https://git.openjdk.org/jdk/pull/12263