Repository: netty/netty
Issue: Http2ConnectionHandler always sends GO_AWAY when channel is closed
Labels: improvement

If the channel is active `Http2ConnectionHandler` will always send a GO_AWAY when the channel is closed. This may be unnecessary, and even confusing to the remote peer, if the local peer is attempting to do a [graceful shutdown](https://tools.ietf.org/html/rfc7540#section-6.8). For example if the local peer manually sends a GO_AWAY to indicate the connection is closing with some `Additional Debug Data` to indicate why, they may also want to send that same debug data in the next GO_AWAY message which carries the real `Last-Stream-ID`. Currently the `Http2ConnectionHandler` will always send a GO_AWAY with empty `Additional Debug Data` when the channel is closing.

One approach to give the user more control is if a GO_AWAY has already been sent when `Http2ConnectionHandler` close's method is called, then we don't send a GO_AWAY, but instead just wait for all the streams to close.


Issue comments:
@nmittler @ejona86 - WDYT?

@Scottmitch Yeah I think that seems reasonable. I suspect we'll want to set some sort of timeout on how long we wait for all of the streams to close before sending a `hard` `GOAWAY` and just closing the connection. Having all of this "pluggable" in some way might be handy.

@Scottmitch SGTM

> I suspect we'll want to set some sort of timeout on how long we wait for all of the streams to close before sending a hard GOAWAY

When close is called we already have a timeout mechanism [gracefulShutdownTimeoutMillis](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L73) which attempts to wait for all streams to be closed before the channel is actually closed. Can you clarify if you think we need more than this, and what you mean by `hard GOAWAY`?

@Scottmitch ah right, I had forgotten about that :).  SGTM


Pull request review comments:
Should the comment above be moved this this `else`?

Maybe add a comment here to describe what you're doing?

I will clarify the comments.

I will clarify the comments.


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
index 3ba341eb456..b5c19d7078d 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
@@ -35,6 +35,7 @@
 import java.util.concurrent.TimeUnit;
 
 import static io.netty.buffer.ByteBufUtil.hexDump;
+import static io.netty.buffer.Unpooled.EMPTY_BUFFER;
 import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_STREAM_ID;
 import static io.netty.handler.codec.http2.Http2CodecUtil.connectionPrefaceBuf;
 import static io.netty.handler.codec.http2.Http2CodecUtil.getEmbeddedHttp2Exception;
@@ -417,17 +418,22 @@ public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exce
             return;
         }
 
-        ChannelFuture future = goAway(ctx, null);
+        // If the user has already sent a GO_AWAY frame they may be attempting to do a graceful shutdown which requires
+        // sending multiple GO_AWAY frames. We should only send a GO_AWAY here if one has not already been sent. If
+        // a GO_AWAY has been sent we send a empty buffer just so we can wait to close until all other data has been
+        // flushed to the OS.
+        // https://github.com/netty/netty/issues/5307
+        final ChannelFuture future = connection().goAwaySent() ? ctx.write(EMPTY_BUFFER) : goAway(ctx, null);
         ctx.flush();
         doGracefulShutdown(ctx, future, promise);
     }
 
     private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, ChannelPromise promise) {
-        // If there are no active streams, close immediately after the send is complete.
-        // Otherwise wait until all streams are inactive.
         if (isGracefulShutdownComplete()) {
+            // If there are no active streams, close immediately after the GO_AWAY write completes.
             future.addListener(new ClosingChannelFutureListener(ctx, promise));
         } else {
+            // If there are active streams we should wait until they are all closed before closing the connection.
             closeListener = new ClosingChannelFutureListener(ctx, promise,
                                         gracefulShutdownTimeoutMillis, MILLISECONDS);
         }


Test patch: