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
This commit is contained in:
Norman Maurer 2019-07-09 21:05:34 +02:00
parent 462e88af7e
commit cd7670dcaa
2 changed files with 46 additions and 8 deletions

View File

@ -475,19 +475,27 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
doGracefulShutdown(ctx, f, promise); 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) { private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, final ChannelPromise promise) {
final ChannelFutureListener listener = newClosingChannelFutureListener(ctx, promise);
if (isGracefulShutdownComplete()) { if (isGracefulShutdownComplete()) {
// If there are no active streams, close immediately after the GO_AWAY write completes. // If there are no active streams, close immediately after the GO_AWAY write completes or the timeout
future.addListener(new ClosingChannelFutureListener(ctx, promise)); // elapsed.
future.addListener(listener);
} else { } else {
// If there are active streams we should wait until they are all closed before closing the connection. // 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 // 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. // new ClosingChannelFutureListener when the graceful close completes if the promise is not null.
if (closeListener == null) { if (closeListener == null) {
closeListener = tmp; closeListener = listener;
} else if (promise != null) { } else if (promise != null) {
final ChannelFutureListener oldCloseListener = closeListener; final ChannelFutureListener oldCloseListener = closeListener;
closeListener = new ChannelFutureListener() { closeListener = new ChannelFutureListener() {
@ -496,7 +504,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
try { try {
oldCloseListener.operationComplete(future); oldCloseListener.operationComplete(future);
} finally { } 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) { if (http2Ex.shutdownHint() == Http2Exception.ShutdownHint.GRACEFUL_SHUTDOWN) {
doGracefulShutdown(ctx, future, promise); doGracefulShutdown(ctx, future, promise);
} else { } else {
future.addListener(new ClosingChannelFutureListener(ctx, promise)); future.addListener(newClosingChannelFutureListener(ctx, promise));
} }
} }

View File

@ -29,6 +29,7 @@ import io.netty.channel.DefaultChannelConfig;
import io.netty.channel.DefaultChannelPromise; import io.netty.channel.DefaultChannelPromise;
import io.netty.handler.codec.http.HttpResponseStatus; import io.netty.handler.codec.http.HttpResponseStatus;
import io.netty.handler.codec.http2.Http2CodecUtil.SimpleChannelPromiseAggregator; 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.ReferenceCountUtil;
import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.EventExecutor;
import io.netty.util.concurrent.GenericFutureListener; import io.netty.util.concurrent.GenericFutureListener;
@ -689,6 +690,25 @@ public class Http2ConnectionHandlerTest {
writeRstStreamUsingVoidPromise(STREAM_ID); 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 @Test
public void gracefulShutdownTimeoutTest() throws Exception { public void gracefulShutdownTimeoutTest() throws Exception {
handler = newHandler(); handler = newHandler();
@ -698,6 +718,16 @@ public class Http2ConnectionHandlerTest {
verify(executor, atLeastOnce()).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS)); 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 @Test
public void gracefulShutdownIndefiniteTimeoutTest() throws Exception { public void gracefulShutdownIndefiniteTimeoutTest() throws Exception {
handler = newHandler(); handler = newHandler();