Http2ConnectionHandler: allow graceful shutdown to wait forever

Motivation:

There should be a way to allow graceful shutdown to wait for all open streams to close without a timeout. Using gracefulShutdownTimeoutMillis with a large value is a bit of a hack, and has a gotcha that sufficiently large values will overflow the long, resulting in a ClosingChannelFutureListener that executes immediately.

Modification:

Allow to use gracefulShutdownTimeoutMillis(-1) to express waiting until all streams are closed.

Result:

We can now shutdown the connection without a forced timeout.

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

@@ -142,7 +142,8 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
     }
 
     /**
-     * Returns the graceful shutdown timeout of the {@link Http2Connection} in milliseconds.
+     * Returns the graceful shutdown timeout of the {@link Http2Connection} in milliseconds. Returns -1 if the
+     * timeout is indefinite.
      */
     protected long gracefulShutdownTimeoutMillis() {
         return gracefulShutdownTimeoutMillis;
@@ -152,6 +153,10 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
      * Sets the graceful shutdown timeout of the {@link Http2Connection} in milliseconds.
      */
     protected B gracefulShutdownTimeoutMillis(long gracefulShutdownTimeoutMillis) {
+        if (gracefulShutdownTimeoutMillis < -1) {
+            throw new IllegalArgumentException("gracefulShutdownTimeoutMillis: " + gracefulShutdownTimeoutMillis +
+                                               " (expected: -1 for indefinite or >= 0)");
+        }
         this.gracefulShutdownTimeoutMillis = gracefulShutdownTimeoutMillis;
         return self();
     }

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

@@ -92,7 +92,8 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
 
     /**
      * Get the amount of time (in milliseconds) this endpoint will wait for all streams to be closed before closing
-     * the connection during the graceful shutdown process.
+     * the connection during the graceful shutdown process. Returns -1 if this connection is configured to wait
+     * indefinitely for all streams to close.
      */
     public long gracefulShutdownTimeoutMillis() {
         return gracefulShutdownTimeoutMillis;
@@ -105,9 +106,9 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
      * streams to be closed before closing the connection during the graceful shutdown process.
      */
     public void gracefulShutdownTimeoutMillis(long gracefulShutdownTimeoutMillis) {
-        if (gracefulShutdownTimeoutMillis < 0) {
+        if (gracefulShutdownTimeoutMillis < -1) {
             throw new IllegalArgumentException("gracefulShutdownTimeoutMillis: " + gracefulShutdownTimeoutMillis +
-                                               " (expected: >= 0)");
+                                               " (expected: -1 for indefinite or >= 0)");
         }
         this.gracefulShutdownTimeoutMillis = gracefulShutdownTimeoutMillis;
     }
@@ -454,8 +455,12 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
             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);
+            if (gracefulShutdownTimeoutMillis < 0) {
+                closeListener = new ClosingChannelFutureListener(ctx, promise);
+            } else {
+                closeListener = new ClosingChannelFutureListener(ctx, promise,
+                                                                 gracefulShutdownTimeoutMillis, MILLISECONDS);
+            }
         }
     }
 

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

@@ -43,6 +43,7 @@ import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 
 import java.util.List;
+import java.util.concurrent.TimeUnit;
 
 import static io.netty.buffer.Unpooled.copiedBuffer;
 import static io.netty.handler.codec.http2.Http2CodecUtil.connectionPrefaceBuf;
@@ -629,6 +630,23 @@ public class Http2ConnectionHandlerTest {
         writeRstStreamUsingVoidPromise(STREAM_ID);
     }
 
+    @Test
+    public void gracefulShutdownTimeoutTest() throws Exception {
+        handler = newHandler();
+        final long expectedMillis = 1234;
+        handler.gracefulShutdownTimeoutMillis(expectedMillis);
+        handler.close(ctx, promise);
+        verify(executor).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
+    }
+
+    @Test
+    public void gracefulShutdownIndefiniteTimeoutTest() throws Exception {
+        handler = newHandler();
+        handler.gracefulShutdownTimeoutMillis(-1);
+        handler.close(ctx, promise);
+        verify(executor, never()).schedule(any(Runnable.class), anyLong(), any(TimeUnit.class));
+    }
+
     private void writeRstStreamUsingVoidPromise(int streamId) throws Exception {
         handler = newHandler();
         final Throwable cause = new RuntimeException("fake exception");