From 643d521d5e21bcfd9d5e1bc3c3a642ecbddb5596 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Thu, 6 Jun 2019 17:44:12 -0700 Subject: [PATCH] HTTP/2 avoid closing connection when writing GOAWAY (#9227) Motivation: b4e3c12b8e8e984ba65330dd6dc34a4b3d07a25a introduced code to avoid coupling close() to graceful close. It also added some code which attempted to infer when a graceful close was being done in writing of a GOAWAY to preserve the "connection is closed when all streams are closed behavior" for the child channel API. However the implementation was too overzealous and may preemptively close the connection if there are not currently any open streams (and close if there are any frames which create streams in flight). Modifications: - Decouple writing a GOAWAY from trying to infer if a graceful close is being done and closing the connection. Even if we could enhance this logic (e.g. wait to close until the second GOAWAY with no error) it is possible the user doesn't want the connection to be closed yet. We can add a means for the codec to orchestrate the graceful close in the future (e.g. write some special "close the connection when all streams are closed") but for now we can just let the application handle this. Result: Fixes https://github.com/netty/netty/issues/9207 --- .../netty/handler/codec/http2/Http2ConnectionHandler.java | 6 ------ .../io/netty/handler/codec/http2/Http2FrameCodecTest.java | 8 ++++---- 2 files changed, 4 insertions(+), 10 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 9306cdf28c..e9726d52ac 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 @@ -836,12 +836,6 @@ 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; } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java index 110bc31d8a..3a38911f2c 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java @@ -330,8 +330,8 @@ public class Http2FrameCodecTest { channel.writeOutbound(goAwayFrame); verify(frameWriter).writeGoAway(eqFrameCodecCtx(), eq(7), eq(NO_ERROR.code()), eq(expected), anyChannelPromise()); - assertEquals(State.CLOSED, stream.state()); - assertFalse(channel.isActive()); + assertEquals(State.OPEN, stream.state()); + assertTrue(channel.isActive()); expected.release(); debugData.release(); } @@ -395,8 +395,8 @@ public class Http2FrameCodecTest { verify(frameWriter).writeGoAway(eqFrameCodecCtx(), eq(Integer.MAX_VALUE), eq(NO_ERROR.code()), eq(debugData), anyChannelPromise()); debugData.release(); - assertEquals(State.CLOSED, stream.state()); - assertFalse(channel.isActive()); + assertEquals(State.OPEN, stream.state()); + assertTrue(channel.isActive()); } @Test