Http2ConnectionHandler to allow decoupling close(..) from GOAWAY graceful close (#9094)

Motivation:
Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close
logic. This coupling means that a user would have to override
Http2ConnectionHandler#close(..) to modify the behavior, and the
Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot
override at this layer. Ideally we can totally decouple the close(..) of the
transport and the GOAWAY graceful closure process completely, but to preserve
backwards compatibility we can add an opt-out option to decouple where the
application is responsible for sending a GOAWAY with error code equal to
NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in
order to initiate graceful close.

Modifications:
- Http2ConnectionHandler supports an additional boolean constructor argument to
opt out of close(..) going through the graceful close path.
- Http2FrameCodecBuilder and Http2MultiplexCodec expose
 gracefulShutdownTimeoutMillis but do not hook them up properly. Since these
are already exposed we should hook them up and make sure the timeout is applied
properly.
- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should
initiate the graceful closure process after writing a GOAWAY frame if the error
code is NO_ERROR. This means that writing a Http2GoAwayFrame from
Http2FrameCodec will initiate graceful close.

Result:
Http2ConnectionHandler#close(..) can now be decoupled from the graceful close
process, and immediately close the underlying transport if desired.

codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandlerBuilder.java

@@ -16,6 +16,7 @@
 
 package io.netty.handler.codec.http2;
 
+import io.netty.channel.Channel;
 import io.netty.handler.codec.http2.Http2HeadersEncoder.SensitivityDetector;
 import io.netty.util.internal.UnstableApi;
 
@@ -83,6 +84,7 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
     private Http2Settings initialSettings = Http2Settings.defaultSettings();
     private Http2FrameListener frameListener;
     private long gracefulShutdownTimeoutMillis = Http2CodecUtil.DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS;
+    private boolean decoupleCloseAndGoAway;
 
     // The property that will prohibit connection() and codec() if set by server(),
     // because this property is used only when this builder creates a Http2Connection.
@@ -401,6 +403,24 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
         return autoAckSettingsFrame;
     }
 
+    /**
+     * Determine if the {@link Channel#close()} should be coupled with goaway and graceful close.
+     * @param decoupleCloseAndGoAway {@code true} to make {@link Channel#close()} directly close the underlying
+     *   transport, and not attempt graceful closure via GOAWAY.
+     * @return {@code this}.
+     */
+    protected B decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
+        this.decoupleCloseAndGoAway = decoupleCloseAndGoAway;
+        return self();
+    }
+
+    /**
+     * Determine if the {@link Channel#close()} should be coupled with goaway and graceful close.
+     */
+    protected boolean decoupleCloseAndGoAway() {
+        return decoupleCloseAndGoAway;
+    }
+
     /**
      * Create a new {@link Http2ConnectionHandler}.
      */

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

@@ -74,15 +74,22 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
     private final Http2ConnectionDecoder decoder;
     private final Http2ConnectionEncoder encoder;
     private final Http2Settings initialSettings;
+    private final boolean decoupleCloseAndGoAway;
     private ChannelFutureListener closeListener;
     private BaseDecoder byteDecoder;
     private long gracefulShutdownTimeoutMillis;
 
     protected Http2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                                      Http2Settings initialSettings) {
+        this(decoder, encoder, initialSettings, false);
+    }
+
+    protected Http2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
+                                     Http2Settings initialSettings, boolean decoupleCloseAndGoAway) {
         this.initialSettings = requireNonNull(initialSettings, "initialSettings");
         this.decoder = requireNonNull(decoder, "decoder");
         this.encoder = requireNonNull(encoder, "encoder");
+        this.decoupleCloseAndGoAway = decoupleCloseAndGoAway;
         if (encoder.connection() != decoder.connection()) {
             throw new IllegalArgumentException("Encoder and Decoder do not share the same connection object");
         }
@@ -447,6 +454,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
 
     @Override
     public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exception {
+        if (decoupleCloseAndGoAway) {
+            ctx.close(promise);
+            return;
+        }
         promise = promise.unvoid();
         // Avoid NotYetConnectedException
         if (!ctx.channel().isActive()) {
@@ -459,22 +470,36 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
         // 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);
+        ChannelFuture f = connection().goAwaySent() ? ctx.write(EMPTY_BUFFER) : goAway(ctx, null, ctx.newPromise());
         ctx.flush();
-        doGracefulShutdown(ctx, future, promise);
+        doGracefulShutdown(ctx, f, promise);
     }
 
-    private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, ChannelPromise promise) {
+    private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, final ChannelPromise promise) {
         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.
-            if (gracefulShutdownTimeoutMillis < 0) {
+            final ClosingChannelFutureListener tmp = gracefulShutdownTimeoutMillis < 0 ?
-                closeListener = new ClosingChannelFutureListener(ctx, promise);
+                    new ClosingChannelFutureListener(ctx, promise) :
-            } else {
+                    new ClosingChannelFutureListener(ctx, promise, gracefulShutdownTimeoutMillis, MILLISECONDS);
-                closeListener = new ClosingChannelFutureListener(ctx, promise,
+            // The ClosingChannelFutureListener will cascade promise completion. We need to always notify the
-                                                                 gracefulShutdownTimeoutMillis, MILLISECONDS);
+            // new ClosingChannelFutureListener when the graceful close completes if the promise is not null.
+            if (closeListener == null) {
+                closeListener = tmp;
+            } else if (promise != null) {
+                final ChannelFutureListener oldCloseListener = closeListener;
+                closeListener = new ChannelFutureListener() {
+                    @Override
+                    public void operationComplete(ChannelFuture future) throws Exception {
+                        try {
+                            oldCloseListener.operationComplete(future);
+                        } finally {
+                            tmp.operationComplete(future);
+                        }
+                    }
+                };
             }
         }
     }
@@ -634,14 +659,11 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
         }
 
         ChannelPromise promise = ctx.newPromise();
-        ChannelFuture future = goAway(ctx, http2Ex);
+        ChannelFuture future = goAway(ctx, http2Ex, ctx.newPromise());
-        switch (http2Ex.shutdownHint()) {
+        if (http2Ex.shutdownHint() == Http2Exception.ShutdownHint.GRACEFUL_SHUTDOWN) {
-        case GRACEFUL_SHUTDOWN:
             doGracefulShutdown(ctx, future, promise);
-            break;
+        } else {
-        default:
             future.addListener(new ClosingChannelFutureListener(ctx, promise));
-            break;
         }
     }
 
@@ -798,6 +820,12 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
             future.addListener((ChannelFutureListener) future1 ->
                     processGoAwayWriteResult(ctx, lastStreamId, errorCode, debugData, future1));
         }
+        // if closeListener != null this means we have already initiated graceful closure. doGracefulShutdown will apply
+        // the gracefulShutdownTimeoutMillis on each invocation, however we only care to apply the timeout on the
+        // start of graceful shutdown.
+        if (errorCode == NO_ERROR.code() && closeListener == null) {
+            doGracefulShutdown(ctx, future, null);
+        }
 
         return future;
     }
@@ -826,10 +854,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
      * Close the remote endpoint with with a {@code GO_AWAY} frame. Does <strong>not</strong> flush
      * immediately, this is the responsibility of the caller.
      */
-    private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause) {
+    private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause, ChannelPromise promise) {
         long errorCode = cause != null ? cause.error().code() : NO_ERROR.code();
         int lastKnownStream = connection().remote().lastStreamCreated();
-        return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), ctx.newPromise());
+        return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), promise);
     }
 
     private void processRstStreamWriteResult(ChannelHandlerContext ctx, Http2Stream stream, ChannelFuture future) {
@@ -898,17 +926,23 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
                                      long timeout, TimeUnit unit) {
             this.ctx = ctx;
             this.promise = promise;
-            timeoutTask = ctx.executor().schedule(() -> {
+            timeoutTask = ctx.executor().schedule(this::doClose, timeout, unit);
-                ctx.close(promise);
-            }, timeout, unit);
         }
 
         @Override
-        public void operationComplete(ChannelFuture sentGoAwayFuture) throws Exception {
+        public void operationComplete(ChannelFuture sentGoAwayFuture) {
             if (timeoutTask != null) {
                 timeoutTask.cancel(false);
             }
-            ctx.close(promise);
+            doClose();
+        }
+
+        private void doClose() {
+            if (promise == null) {
+                ctx.close();
+            } else {
+                ctx.close(promise);
+            }
         }
     }
 }

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandlerBuilder.java

@@ -92,6 +92,11 @@ public final class Http2ConnectionHandlerBuilder
         return super.initialHuffmanDecodeCapacity(initialHuffmanDecodeCapacity);
     }
 
+    @Override
+    public Http2ConnectionHandlerBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
+        return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
+    }
+
     @Override
     public Http2ConnectionHandler build() {
         return super.build();
@@ -100,6 +105,6 @@ public final class Http2ConnectionHandlerBuilder
     @Override
     protected Http2ConnectionHandler build(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                                            Http2Settings initialSettings) {
-        return new Http2ConnectionHandler(decoder, encoder, initialSettings);
+        return new Http2ConnectionHandler(decoder, encoder, initialSettings, decoupleCloseAndGoAway());
     }
 }

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java

@@ -159,8 +159,9 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
     private final IntObjectMap<DefaultHttp2FrameStream> frameStreamToInitializeMap =
             new IntObjectHashMap<DefaultHttp2FrameStream>(8);
 
-    Http2FrameCodec(Http2ConnectionEncoder encoder, Http2ConnectionDecoder decoder, Http2Settings initialSettings) {
+    Http2FrameCodec(Http2ConnectionEncoder encoder, Http2ConnectionDecoder decoder, Http2Settings initialSettings,
-        super(decoder, encoder, initialSettings);
+                    boolean decoupleCloseAndGoAway) {
+        super(decoder, encoder, initialSettings, decoupleCloseAndGoAway);
 
         decoder.frameListener(new FrameListener());
         connection().addListener(new ConnectionListener());
@@ -496,7 +497,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
     void onHttp2UnknownStreamError(@SuppressWarnings("unused") ChannelHandlerContext ctx, Throwable cause,
                                    Http2Exception.StreamException streamException) {
         // Just log....
-        LOG.warn("Stream exception thrown for unkown stream {}.", streamException.streamId(), cause);
+        LOG.warn("Stream exception thrown for unknown stream {}.", streamException.streamId(), cause);
     }
 
     @Override

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java

@@ -31,6 +31,8 @@ public class Http2FrameCodecBuilder extends
 
     Http2FrameCodecBuilder(boolean server) {
         server(server);
+        // For backwards compatibility we should disable to timeout by default at this layer.
+        gracefulShutdownTimeoutMillis(0);
     }
 
     /**
@@ -139,6 +141,11 @@ public class Http2FrameCodecBuilder extends
         return super.initialHuffmanDecodeCapacity(initialHuffmanDecodeCapacity);
     }
 
+    @Override
+    public Http2FrameCodecBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
+        return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
+    }
+
     /**
      * Build a {@link Http2FrameCodec} object.
      */
@@ -173,6 +180,8 @@ public class Http2FrameCodecBuilder extends
     @Override
     protected Http2FrameCodec build(
             Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings) {
-        return new Http2FrameCodec(encoder, decoder, initialSettings);
+        Http2FrameCodec codec = new Http2FrameCodec(encoder, decoder, initialSettings, decoupleCloseAndGoAway());
+        codec.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis());
+        return codec;
     }
 }

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java

@@ -155,8 +155,8 @@ public class Http2MultiplexCodec extends Http2FrameCodec {
                         Http2ConnectionDecoder decoder,
                         Http2Settings initialSettings,
                         ChannelHandler inboundStreamHandler,
-                        ChannelHandler upgradeStreamHandler) {
+                        ChannelHandler upgradeStreamHandler, boolean decoupleCloseAndGoAway) {
-        super(encoder, decoder, initialSettings);
+        super(encoder, decoder, initialSettings, decoupleCloseAndGoAway);
         this.inboundStreamHandler = inboundStreamHandler;
         this.upgradeStreamHandler = upgradeStreamHandler;
     }

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java

@@ -35,6 +35,8 @@ public class Http2MultiplexCodecBuilder
     Http2MultiplexCodecBuilder(boolean server, ChannelHandler childHandler) {
         server(server);
         this.childHandler = checkSharable(requireNonNull(childHandler, "childHandler"));
+        // For backwards compatibility we should disable to timeout by default at this layer.
+        gracefulShutdownTimeoutMillis(0);
     }
 
     private static ChannelHandler checkSharable(ChannelHandler handler) {
@@ -71,6 +73,14 @@ public class Http2MultiplexCodecBuilder
         return new Http2MultiplexCodecBuilder(true, childHandler);
     }
 
+    public Http2MultiplexCodecBuilder withUpgradeStreamHandler(ChannelHandler upgradeStreamHandler) {
+        if (this.isServer()) {
+            throw new IllegalArgumentException("Server codecs don't use an extra handler for the upgrade stream");
+        }
+        this.upgradeStreamHandler = upgradeStreamHandler;
+        return this;
+    }
+
     @Override
     public Http2Settings initialSettings() {
         return super.initialSettings();
@@ -91,14 +101,6 @@ public class Http2MultiplexCodecBuilder
         return super.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis);
     }
 
-    public Http2MultiplexCodecBuilder withUpgradeStreamHandler(ChannelHandler upgradeStreamHandler) {
-        if (this.isServer()) {
-            throw new IllegalArgumentException("Server codecs don't use an extra handler for the upgrade stream");
-        }
-        this.upgradeStreamHandler = upgradeStreamHandler;
-        return this;
-    }
-
     @Override
     public boolean isServer() {
         return super.isServer();
@@ -170,6 +172,11 @@ public class Http2MultiplexCodecBuilder
         return super.autoAckSettingsFrame(autoAckSettings);
     }
 
+    @Override
+    public Http2MultiplexCodecBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
+        return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
+    }
+
     @Override
     public Http2MultiplexCodec build() {
         Http2FrameWriter frameWriter = this.frameWriter;
@@ -201,6 +208,9 @@ public class Http2MultiplexCodecBuilder
     @Override
     protected Http2MultiplexCodec build(
             Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings) {
-        return new Http2MultiplexCodec(encoder, decoder, initialSettings, childHandler, upgradeStreamHandler);
+        Http2MultiplexCodec codec = new Http2MultiplexCodec(encoder, decoder, initialSettings, childHandler,
+                upgradeStreamHandler, decoupleCloseAndGoAway());
+        codec.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis());
+        return codec;
     }
 }

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java

@@ -45,6 +45,13 @@ public class HttpToHttp2ConnectionHandler extends Http2ConnectionHandler {
         this.validateHeaders = validateHeaders;
     }
 
+    protected HttpToHttp2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
+                                           Http2Settings initialSettings, boolean validateHeaders,
+                                           boolean decoupleCloseAndGoAway) {
+        super(decoder, encoder, initialSettings, decoupleCloseAndGoAway);
+        this.validateHeaders = validateHeaders;
+    }
+
     /**
      * Get the next stream id either from the {@link HttpHeaders} object or HTTP/2 codec
      *

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerBuilder.java

@@ -84,6 +84,11 @@ public final class HttpToHttp2ConnectionHandlerBuilder extends
         return super.initialHuffmanDecodeCapacity(initialHuffmanDecodeCapacity);
     }
 
+    @Override
+    public HttpToHttp2ConnectionHandlerBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
+        return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
+    }
+
     @Override
     public HttpToHttp2ConnectionHandler build() {
         return super.build();
@@ -92,6 +97,7 @@ public final class HttpToHttp2ConnectionHandlerBuilder extends
     @Override
     protected HttpToHttp2ConnectionHandler build(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                                                  Http2Settings initialSettings) {
-        return new HttpToHttp2ConnectionHandler(decoder, encoder, initialSettings, isValidateHeaders());
+        return new HttpToHttp2ConnectionHandler(decoder, encoder, initialSettings, isValidateHeaders(),
+                decoupleCloseAndGoAway());
     }
 }

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java

@@ -695,7 +695,7 @@ public class Http2ConnectionHandlerTest {
         final long expectedMillis = 1234;
         handler.gracefulShutdownTimeoutMillis(expectedMillis);
         handler.close(ctx, promise);
-        verify(executor).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
+        verify(executor, atLeastOnce()).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
     }
 
     @Test

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java

@@ -219,7 +219,7 @@ public class Http2FrameCodecTest {
         Http2Connection conn = new DefaultHttp2Connection(true);
         Http2ConnectionEncoder enc = new DefaultHttp2ConnectionEncoder(conn, new DefaultHttp2FrameWriter());
         Http2ConnectionDecoder dec = new DefaultHttp2ConnectionDecoder(conn, enc, new DefaultHttp2FrameReader());
-        Http2FrameCodec codec = new Http2FrameCodec(enc, dec, new Http2Settings());
+        Http2FrameCodec codec = new Http2FrameCodec(enc, dec, new Http2Settings(), false);
         EmbeddedChannel em = new EmbeddedChannel(codec);
 
         // We call #consumeBytes on a stream id which has not been seen yet to emulate the case
@@ -323,15 +323,15 @@ public class Http2FrameCodecTest {
         ByteBuf debugData = bb("debug");
         ByteBuf expected = debugData.copy();
 
-        Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(), debugData);
+        Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(),
+                debugData.retainedDuplicate());
         goAwayFrame.setExtraStreamIds(2);
 
         channel.writeOutbound(goAwayFrame);
         verify(frameWriter).writeGoAway(eqFrameCodecCtx(), eq(7),
                 eq(NO_ERROR.code()), eq(expected), anyChannelPromise());
-        assertEquals(1, debugData.refCnt());
+        assertEquals(State.CLOSED, stream.state());
-        assertEquals(State.OPEN, stream.state());
+        assertFalse(channel.isActive());
-        assertTrue(channel.isActive());
         expected.release();
         debugData.release();
     }
@@ -386,16 +386,17 @@ public class Http2FrameCodecTest {
         assertEquals(State.OPEN, stream.state());
 
         ByteBuf debugData = bb("debug");
-        Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(), debugData.slice());
+        Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(),
+                debugData.retainedDuplicate());
         goAwayFrame.setExtraStreamIds(Integer.MAX_VALUE);
 
         channel.writeOutbound(goAwayFrame);
         // When the last stream id computation overflows, the last stream id should just be set to 2^31 - 1.
         verify(frameWriter).writeGoAway(eqFrameCodecCtx(), eq(Integer.MAX_VALUE),
                 eq(NO_ERROR.code()), eq(debugData), anyChannelPromise());
-        assertEquals(1, debugData.refCnt());
+        debugData.release();
-        assertEquals(State.OPEN, stream.state());
+        assertEquals(State.CLOSED, stream.state());
-        assertTrue(channel.isActive());
+        assertFalse(channel.isActive());
     }
 
     @Test