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

Http2ConnectionHandler to allow decoupling close(..) from GOAWAY graceful close (#9094)

Browse Source 
Motivation:
Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close
logic. This coupling means that a user would have to override
Http2ConnectionHandler#close(..) to modify the behavior, and the
Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot
override at this layer. Ideally we can totally decouple the close(..) of the
transport and the GOAWAY graceful closure process completely, but to preserve
backwards compatibility we can add an opt-out option to decouple where the
application is responsible for sending a GOAWAY with error code equal to
NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in
order to initiate graceful close.

Modifications:
- Http2ConnectionHandler supports an additional boolean constructor argument to
opt out of close(..) going through the graceful close path.
- Http2FrameCodecBuilder and Http2MultiplexCodec expose
 gracefulShutdownTimeoutMillis but do not hook them up properly. Since these
are already exposed we should hook them up and make sure the timeout is applied
properly.
- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should
initiate the graceful closure process after writing a GOAWAY frame if the error
code is NO_ERROR. This means that writing a Http2GoAwayFrame from
Http2FrameCodec will initiate graceful close.

Result:
Http2ConnectionHandler#close(..) can now be decoupled from the graceful close
process, and immediately close the underlying transport if desired.
This commit is contained in:
Scott Mitchell 2019-04-28 17:48:04 -07:00 committed by GitHub
parent 00a9a25f29
commit b4e3c12b8e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 141 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -16,6 +16,7 @@
package io.netty.handler.codec.http2;
import io.netty.channel.Channel;
import io.netty.handler.codec.http2.Http2HeadersEncoder.SensitivityDetector;
import io.netty.util.internal.UnstableApi;
@ -83,6 +84,7 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
private Http2Settings initialSettings = Http2Settings.defaultSettings();
private Http2FrameListener frameListener;
private long gracefulShutdownTimeoutMillis = Http2CodecUtil.DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS;
private boolean decoupleCloseAndGoAway;
// The property that will prohibit connection() and codec() if set by server(),
// because this property is used only when this builder creates a Http2Connection.
@ -401,6 +403,24 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
return autoAckSettingsFrame;
}
/**
* Determine if the {@link Channel#close()} should be coupled with goaway and graceful close.
* @param decoupleCloseAndGoAway {@code true} to make {@link Channel#close()} directly close the underlying
* transport, and not attempt graceful closure via GOAWAY.
* @return {@code this}.
*/
protected B decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
this.decoupleCloseAndGoAway = decoupleCloseAndGoAway;
return self();
}
/**
* Determine if the {@link Channel#close()} should be coupled with goaway and graceful close.
*/
protected boolean decoupleCloseAndGoAway() {
return decoupleCloseAndGoAway;
}
/**
* Create a new {@link Http2ConnectionHandler}.
*/

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

@ -76,15 +76,22 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
private final Http2ConnectionDecoder decoder;
private final Http2ConnectionEncoder encoder;
private final Http2Settings initialSettings;
private final boolean decoupleCloseAndGoAway;
private ChannelFutureListener closeListener;
private BaseDecoder byteDecoder;
private long gracefulShutdownTimeoutMillis;
protected Http2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings) {
this(decoder, encoder, initialSettings, false);
}
protected Http2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings, boolean decoupleCloseAndGoAway) {
this.initialSettings = checkNotNull(initialSettings, "initialSettings");
this.decoder = checkNotNull(decoder, "decoder");
this.encoder = checkNotNull(encoder, "encoder");
this.decoupleCloseAndGoAway = decoupleCloseAndGoAway;
if (encoder.connection() != decoder.connection()) {
throw new IllegalArgumentException("Encoder and Decoder do not share the same connection object");
}
@ -449,6 +456,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
@Override
public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exception {
if (decoupleCloseAndGoAway) {
ctx.close(promise);
return;
}
promise = promise.unvoid();
// Avoid NotYetConnectedException
if (!ctx.channel().isActive()) {
@ -461,22 +472,36 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
// a GO_AWAY has been sent we send a empty buffer just so we can wait to close until all other data has been
// flushed to the OS.
// https://github.com/netty/netty/issues/5307
final ChannelFuture future = connection().goAwaySent() ? ctx.write(EMPTY_BUFFER) : goAway(ctx, null);
ChannelFuture f = connection().goAwaySent() ? ctx.write(EMPTY_BUFFER) : goAway(ctx, null, ctx.newPromise());
ctx.flush();
doGracefulShutdown(ctx, future, promise);
doGracefulShutdown(ctx, f, promise);
}
private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, ChannelPromise promise) {
private void doGracefulShutdown(ChannelHandlerContext ctx, ChannelFuture future, final ChannelPromise promise) {
if (isGracefulShutdownComplete()) {
// If there are no active streams, close immediately after the GO_AWAY write completes.
future.addListener(new ClosingChannelFutureListener(ctx, promise));
} else {
// If there are active streams we should wait until they are all closed before closing the connection.
if (gracefulShutdownTimeoutMillis < 0) {
closeListener = new ClosingChannelFutureListener(ctx, promise);
} else {
closeListener = new ClosingChannelFutureListener(ctx, promise,
gracefulShutdownTimeoutMillis, MILLISECONDS);
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;
} else if (promise != null) {
final ChannelFutureListener oldCloseListener = closeListener;
closeListener = new ChannelFutureListener() {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
try {
oldCloseListener.operationComplete(future);
} finally {
tmp.operationComplete(future);
}
}
};
}
}
}
@ -636,14 +661,11 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
}
ChannelPromise promise = ctx.newPromise();
ChannelFuture future = goAway(ctx, http2Ex);
switch (http2Ex.shutdownHint()) {
case GRACEFUL_SHUTDOWN:
ChannelFuture future = goAway(ctx, http2Ex, ctx.newPromise());
if (http2Ex.shutdownHint() == Http2Exception.ShutdownHint.GRACEFUL_SHUTDOWN) {
doGracefulShutdown(ctx, future, promise);
break;
default:
} else {
future.addListener(new ClosingChannelFutureListener(ctx, promise));
break;
}
}
@ -814,6 +836,12 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
}
});
}
// if closeListener != null this means we have already initiated graceful closure. doGracefulShutdown will apply
// the gracefulShutdownTimeoutMillis on each invocation, however we only care to apply the timeout on the
// start of graceful shutdown.
if (errorCode == NO_ERROR.code() && closeListener == null) {
doGracefulShutdown(ctx, future, null);
}
return future;
}
@ -842,10 +870,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
* Close the remote endpoint with with a {@code GO_AWAY} frame. Does <strong>not</strong> flush
* immediately, this is the responsibility of the caller.
*/
private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause) {
private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause, ChannelPromise promise) {
long errorCode = cause != null ? cause.error().code() : NO_ERROR.code();
int lastKnownStream = connection().remote().lastStreamCreated();
return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), ctx.newPromise());
return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), promise);
}
private void processRstStreamWriteResult(ChannelHandlerContext ctx, Http2Stream stream, ChannelFuture future) {
@ -917,17 +945,25 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
timeoutTask = ctx.executor().schedule(new Runnable() {
@Override
public void run() {
ctx.close(promise);
doClose();
}
}, timeout, unit);
}
@Override
public void operationComplete(ChannelFuture sentGoAwayFuture) throws Exception {
public void operationComplete(ChannelFuture sentGoAwayFuture) {
if (timeoutTask != null) {
timeoutTask.cancel(false);
}
ctx.close(promise);
doClose();
}
private void doClose() {
if (promise == null) {
ctx.close();
} else {
ctx.close(promise);
}
}
}
}

7
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandlerBuilder.java
View File

@ -92,6 +92,11 @@ public final class Http2ConnectionHandlerBuilder
return super.initialHuffmanDecodeCapacity(initialHuffmanDecodeCapacity);
}
@Override
public Http2ConnectionHandlerBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
}
@Override
public Http2ConnectionHandler build() {
return super.build();
@ -100,6 +105,6 @@ public final class Http2ConnectionHandlerBuilder
@Override
protected Http2ConnectionHandler build(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings) {
return new Http2ConnectionHandler(decoder, encoder, initialSettings);
return new Http2ConnectionHandler(decoder, encoder, initialSettings, decoupleCloseAndGoAway());
}
}

7
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
View File

@ -159,8 +159,9 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
private final IntObjectMap<DefaultHttp2FrameStream> frameStreamToInitializeMap =
new IntObjectHashMap<DefaultHttp2FrameStream>(8);
Http2FrameCodec(Http2ConnectionEncoder encoder, Http2ConnectionDecoder decoder, Http2Settings initialSettings) {
super(decoder, encoder, initialSettings);
Http2FrameCodec(Http2ConnectionEncoder encoder, Http2ConnectionDecoder decoder, Http2Settings initialSettings,
boolean decoupleCloseAndGoAway) {
super(decoder, encoder, initialSettings, decoupleCloseAndGoAway);
decoder.frameListener(new FrameListener());
connection().addListener(new ConnectionListener());
@ -502,7 +503,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
void onHttp2UnknownStreamError(@SuppressWarnings("unused") ChannelHandlerContext ctx, Throwable cause,
Http2Exception.StreamException streamException) {
// Just log....
LOG.warn("Stream exception thrown for unkown stream {}.", streamException.streamId(), cause);
LOG.warn("Stream exception thrown for unknown stream {}.", streamException.streamId(), cause);
}
@Override

11
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java
View File

@ -31,6 +31,8 @@ public class Http2FrameCodecBuilder extends
Http2FrameCodecBuilder(boolean server) {
server(server);
// For backwards compatibility we should disable to timeout by default at this layer.
gracefulShutdownTimeoutMillis(0);
}
/**
@ -139,6 +141,11 @@ public class Http2FrameCodecBuilder extends
return super.initialHuffmanDecodeCapacity(initialHuffmanDecodeCapacity);
}
@Override
public Http2FrameCodecBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
}
/**
* Build a {@link Http2FrameCodec} object.
*/
@ -173,6 +180,8 @@ public class Http2FrameCodecBuilder extends
@Override
protected Http2FrameCodec build(
Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings) {
return new Http2FrameCodec(encoder, decoder, initialSettings);
Http2FrameCodec codec = new Http2FrameCodec(encoder, decoder, initialSettings, decoupleCloseAndGoAway());
codec.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis());
return codec;
}
}

4
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java
View File

@ -165,8 +165,8 @@ public class Http2MultiplexCodec extends Http2FrameCodec {
Http2ConnectionDecoder decoder,
Http2Settings initialSettings,
ChannelHandler inboundStreamHandler,
ChannelHandler upgradeStreamHandler) {
super(encoder, decoder, initialSettings);
ChannelHandler upgradeStreamHandler, boolean decoupleCloseAndGoAway) {
super(encoder, decoder, initialSettings, decoupleCloseAndGoAway);
this.inboundStreamHandler = inboundStreamHandler;
this.upgradeStreamHandler = upgradeStreamHandler;
}

28
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
View File

@ -35,6 +35,8 @@ public class Http2MultiplexCodecBuilder
Http2MultiplexCodecBuilder(boolean server, ChannelHandler childHandler) {
server(server);
this.childHandler = checkSharable(checkNotNull(childHandler, "childHandler"));
// For backwards compatibility we should disable to timeout by default at this layer.
gracefulShutdownTimeoutMillis(0);
}
private static ChannelHandler checkSharable(ChannelHandler handler) {
@ -71,6 +73,14 @@ public class Http2MultiplexCodecBuilder
return new Http2MultiplexCodecBuilder(true, childHandler);
}
public Http2MultiplexCodecBuilder withUpgradeStreamHandler(ChannelHandler upgradeStreamHandler) {
if (this.isServer()) {
throw new IllegalArgumentException("Server codecs don't use an extra handler for the upgrade stream");
}
this.upgradeStreamHandler = upgradeStreamHandler;
return this;
}
@Override
public Http2Settings initialSettings() {
return super.initialSettings();
@ -91,14 +101,6 @@ public class Http2MultiplexCodecBuilder
return super.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis);
}
public Http2MultiplexCodecBuilder withUpgradeStreamHandler(ChannelHandler upgradeStreamHandler) {
if (this.isServer()) {
throw new IllegalArgumentException("Server codecs don't use an extra handler for the upgrade stream");
}
this.upgradeStreamHandler = upgradeStreamHandler;
return this;
}
@Override
public boolean isServer() {
return super.isServer();
@ -170,6 +172,11 @@ public class Http2MultiplexCodecBuilder
return super.autoAckSettingsFrame(autoAckSettings);
}
@Override
public Http2MultiplexCodecBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
}
@Override
public Http2MultiplexCodec build() {
Http2FrameWriter frameWriter = this.frameWriter;
@ -201,6 +208,9 @@ public class Http2MultiplexCodecBuilder
@Override
protected Http2MultiplexCodec build(
Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings) {
return new Http2MultiplexCodec(encoder, decoder, initialSettings, childHandler, upgradeStreamHandler);
Http2MultiplexCodec codec = new Http2MultiplexCodec(encoder, decoder, initialSettings, childHandler,
upgradeStreamHandler, decoupleCloseAndGoAway());
codec.gracefulShutdownTimeoutMillis(gracefulShutdownTimeoutMillis());
return codec;
}
}

7
codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java
View File

@ -45,6 +45,13 @@ public class HttpToHttp2ConnectionHandler extends Http2ConnectionHandler {
this.validateHeaders = validateHeaders;
}
protected HttpToHttp2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings, boolean validateHeaders,
boolean decoupleCloseAndGoAway) {
super(decoder, encoder, initialSettings, decoupleCloseAndGoAway);
this.validateHeaders = validateHeaders;
}
/**
* Get the next stream id either from the {@link HttpHeaders} object or HTTP/2 codec
*

8
codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerBuilder.java
View File

@ -84,6 +84,11 @@ public final class HttpToHttp2ConnectionHandlerBuilder extends
return super.initialHuffmanDecodeCapacity(initialHuffmanDecodeCapacity);
}
@Override
public HttpToHttp2ConnectionHandlerBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
}
@Override
public HttpToHttp2ConnectionHandler build() {
return super.build();
@ -92,6 +97,7 @@ public final class HttpToHttp2ConnectionHandlerBuilder extends
@Override
protected HttpToHttp2ConnectionHandler build(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings) {
return new HttpToHttp2ConnectionHandler(decoder, encoder, initialSettings, isValidateHeaders());
return new HttpToHttp2ConnectionHandler(decoder, encoder, initialSettings, isValidateHeaders(),
decoupleCloseAndGoAway());
}
}

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

@ -727,7 +727,7 @@ public class Http2ConnectionHandlerTest {
final long expectedMillis = 1234;
handler.gracefulShutdownTimeoutMillis(expectedMillis);
handler.close(ctx, promise);
verify(executor).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
verify(executor, atLeastOnce()).schedule(any(Runnable.class), eq(expectedMillis), eq(TimeUnit.MILLISECONDS));
}
@Test

19
codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
View File

@ -219,7 +219,7 @@ public class Http2FrameCodecTest {
Http2Connection conn = new DefaultHttp2Connection(true);
Http2ConnectionEncoder enc = new DefaultHttp2ConnectionEncoder(conn, new DefaultHttp2FrameWriter());
Http2ConnectionDecoder dec = new DefaultHttp2ConnectionDecoder(conn, enc, new DefaultHttp2FrameReader());
Http2FrameCodec codec = new Http2FrameCodec(enc, dec, new Http2Settings());
Http2FrameCodec codec = new Http2FrameCodec(enc, dec, new Http2Settings(), false);
EmbeddedChannel em = new EmbeddedChannel(codec);
// We call #consumeBytes on a stream id which has not been seen yet to emulate the case
@ -323,15 +323,15 @@ public class Http2FrameCodecTest {
ByteBuf debugData = bb("debug");
ByteBuf expected = debugData.copy();
Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(), debugData);
Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(),
debugData.retainedDuplicate());
goAwayFrame.setExtraStreamIds(2);
channel.writeOutbound(goAwayFrame);
verify(frameWriter).writeGoAway(eqFrameCodecCtx(), eq(7),
eq(NO_ERROR.code()), eq(expected), anyChannelPromise());
assertEquals(1, debugData.refCnt());
assertEquals(State.OPEN, stream.state());
assertTrue(channel.isActive());
assertEquals(State.CLOSED, stream.state());
assertFalse(channel.isActive());
expected.release();
debugData.release();
}
@ -386,16 +386,17 @@ public class Http2FrameCodecTest {
assertEquals(State.OPEN, stream.state());
ByteBuf debugData = bb("debug");
Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(), debugData.slice());
Http2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(NO_ERROR.code(),
debugData.retainedDuplicate());
goAwayFrame.setExtraStreamIds(Integer.MAX_VALUE);
channel.writeOutbound(goAwayFrame);
// When the last stream id computation overflows, the last stream id should just be set to 2^31 - 1.
verify(frameWriter).writeGoAway(eqFrameCodecCtx(), eq(Integer.MAX_VALUE),
eq(NO_ERROR.code()), eq(debugData), anyChannelPromise());
assertEquals(1, debugData.refCnt());
assertEquals(State.OPEN, stream.state());
assertTrue(channel.isActive());
debugData.release();
assertEquals(State.CLOSED, stream.state());
assertFalse(channel.isActive());
}
@Test