andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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

Browse Source 
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 committed by GitHub
parent 4312e11316
commit bded2a1c75
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 46 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -477,19 +477,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() {
@ -498,7 +506,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
try {
oldCloseListener.operationComplete(future);
} finally {
tmp.operationComplete(future);
listener.operationComplete(future);
}
}
};
@ -665,7 +673,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));
}
}

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

@ -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;
@ -721,6 +722,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();
@ -730,6 +750,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();