HTTP2: Always apply the graceful shutdown timeout if configured (netty#9340)

Motivation:

Http2ConnectionHandler (and sub-classes) allow to configure a graceful shutdown timeout but only apply it if there is at least one active stream. We should always apply the timeout. This is also true when we try to send a GO_AWAY and close the connection because of an connection error.

Modifications:

- Always apply the timeout if one is configured
- Add unit test

Result:

Always respect gracefulShutdownTimeoutMillis

Author: normanmaurer

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

@@ -477,19 +477,27 @@ public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exce
         doGracefulShutdown(ctx, f, promise);
     }
 
+    private ChannelFutureListener newClosingChannelFutureListener(
+            ChannelHandlerContext ctx, ChannelPromise promise) {
+        long gracefulShutdownTimeoutMillis = this.gracefulShutdownTimeoutMillis;
+        return gracefulShutdownTimeoutMillis < 0 ?
+                new ClosingChannelFutureListener(ctx, promise) :
+                new ClosingChannelFutureListener(ctx, promise, gracefulShutdownTimeoutMillis, MILLISECONDS);
+    }
+
     private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, final ChannelPromise promise) {
+        final ChannelFutureListener listener = newClosingChannelFutureListener(ctx, promise);
         if (isGracefulShutdownComplete()) {
-            // If there are no active streams, close immediately after the GO_AWAY write completes.
-            future.addListener(new ClosingChannelFutureListener(ctx, promise));
+            // If there are no active streams, close immediately after the GO_AWAY write completes or the timeout
+            // elapsed.
+            future.addListener(listener);
         } else {
             // If there are active streams we should wait until they are all closed before closing the connection.
-            final ClosingChannelFutureListener tmp = gracefulShutdownTimeoutMillis < 0 ?
-                    new ClosingChannelFutureListener(ctx, promise) :
-                    new ClosingChannelFutureListener(ctx, promise, gracefulShutdownTimeoutMillis, MILLISECONDS);
+
             // The ClosingChannelFutureListener will cascade promise completion. We need to always notify the
             // new ClosingChannelFutureListener when the graceful close completes if the promise is not null.
             if (closeListener == null) {
-                closeListener = tmp;
+                closeListener = listener;
             } else if (promise != null) {
                 final ChannelFutureListener oldCloseListener = closeListener;
                 closeListener = new ChannelFutureListener() {
@@ -498,7 +506,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
                         try {
                             oldCloseListener.operationComplete(future);
                         } finally {
-                            tmp.operationComplete(future);
+                            listener.operationComplete(future);
                         }
                     }
                 };
@@ -665,7 +673,7 @@ protected void onConnectionError(ChannelHandlerContext ctx, boolean outbound,
         if (http2Ex.shutdownHint() == Http2Exception.ShutdownHint.GRACEFUL_SHUTDOWN) {
             doGracefulShutdown(ctx, future, promise);
         } else {
-            future.addListener(new ClosingChannelFutureListener(ctx, promise));
+            future.addListener(newClosingChannelFutureListener(ctx, promise));
         }
     }
 

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

@@ -29,6 +29,7 @@
 import io.netty.channel.DefaultChannelPromise;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http2.Http2CodecUtil.SimpleChannelPromiseAggregator;
+import io.netty.handler.codec.http2.Http2Exception.ShutdownHint;
 import io.netty.util.ReferenceCountUtil;
 import io.netty.util.concurrent.EventExecutor;
 import io.netty.util.concurrent.GenericFutureListener;
@@ -721,6 +722,25 @@ public void writeRstStreamForKnownStreamUsingVoidPromise() throws Exception {
         writeRstStreamUsingVoidPromise(STREAM_ID);
     }
 
+    @Test
+    public void gracefulShutdownTimeoutWhenConnectionErrorHardShutdownTest() throws Exception {
+        gracefulShutdownTimeoutWhenConnectionErrorTest0(ShutdownHint.HARD_SHUTDOWN);
+    }
+
+    @Test
+    public void gracefulShutdownTimeoutWhenConnectionErrorGracefulShutdownTest() throws Exception {
+        gracefulShutdownTimeoutWhenConnectionErrorTest0(ShutdownHint.GRACEFUL_SHUTDOWN);
+    }
+
+    private void gracefulShutdownTimeoutWhenConnectionErrorTest0(ShutdownHint hint) throws Exception {
+        handler = newHandler();
+        final long expectedMillis = 1234;
+        handler.gracefulShutdownTimeoutMillis(expectedMillis);
+        Http2Exception exception = new Http2Exception(PROTOCOL_ERROR, "Test error", hint);
+        handler.onConnectionError(ctx, false, exception, exception);
+        verify(executor, atLeastOnce()).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
+    }
+
     @Test
     public void gracefulShutdownTimeoutTest() throws Exception {
         handler = newHandler();
@@ -730,6 +750,16 @@ public void gracefulShutdownTimeoutTest() throws Exception {
         verify(executor, atLeastOnce()).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
     }
 
+    @Test
+    public void gracefulShutdownTimeoutNoActiveStreams() throws Exception {
+        handler = newHandler();
+        when(connection.numActiveStreams()).thenReturn(0);
+        final long expectedMillis = 1234;
+        handler.gracefulShutdownTimeoutMillis(expectedMillis);
+        handler.close(ctx, promise);
+        verify(executor, atLeastOnce()).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
+    }
+
     @Test
     public void gracefulShutdownIndefiniteTimeoutTest() throws Exception {
         handler = newHandler();