From e0bfc4f91f9ccf17f3e852c2356f7ac2aa6d47ea 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 5243ed84d6..8e30d40fc0 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 @@ -820,12 +820,6 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http future.addListener((ChannelFutureListener) future1 -> processGoAwayWriteResult(ctx, lastStreamId, errorCode, debugData, future1)); } - // 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 7b3ef43442..dd8f822554 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