From cd7670dcaaae559e0bd3b57c360e97029d8c45bb Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 9 Jul 2019 21:05:34 +0200 Subject: [PATCH] HTTP2: Always apply the graceful shutdown timeout if configured (#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 --- .../codec/http2/Http2ConnectionHandler.java | 24 ++++++++++----- .../http2/Http2ConnectionHandlerTest.java | 30 +++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) 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 8e30d40fc0..c95b53b058 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 @@ -475,19 +475,27 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http 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() { @@ -496,7 +504,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http try { oldCloseListener.operationComplete(future); } finally { - tmp.operationComplete(future); + listener.operationComplete(future); } } }; @@ -663,7 +671,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http if (http2Ex.shutdownHint() == Http2Exception.ShutdownHint.GRACEFUL_SHUTDOWN) { doGracefulShutdown(ctx, future, promise); } else { - future.addListener(new ClosingChannelFutureListener(ctx, promise)); + future.addListener(newClosingChannelFutureListener(ctx, promise)); } } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java index fceb820da0..eba3ee455d 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java @@ -29,6 +29,7 @@ import io.netty.channel.DefaultChannelConfig; 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; @@ -689,6 +690,25 @@ public class Http2ConnectionHandlerTest { 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(); @@ -698,6 +718,16 @@ public class Http2ConnectionHandlerTest { 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();