From af83b56e59d6f6ee0423ea140cddb5c635367ceb Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Wed, 18 Mar 2015 14:09:19 -0700 Subject: [PATCH] HTTP2: Close encoder and decoder on channelInactive and initialize clientPrefaceString on handlerAdded. Motivation: - The encoder and decoder should be closed right after the handler releases its resources. - The clientPrefaceString is allocated in the constructor but releases in handlerRemoved. If the handler is never added to the pipeline, the clientPrefaceString will never be released. Modifications: - Call encoder.close() and decoder.close() on channelInactive. - Release the clientPrefaceString on handlerRemoved. Result: - The encoder and decoder get closed right after the handler's resources are freed. - It's easier to verify that the clientPrefaceString will also get released. --- .../codec/http2/Http2ConnectionHandler.java | 40 ++++++++++--------- .../http2/Http2ConnectionHandlerTest.java | 31 +++++++++++--- 2 files changed, 47 insertions(+), 24 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 1b4e974786..bb850792ff 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 @@ -15,6 +15,7 @@ package io.netty.handler.codec.http2; import static io.netty.handler.codec.http2.Http2CodecUtil.HTTP_UPGRADE_STREAM_ID; +import static io.netty.handler.codec.http2.Http2CodecUtil.PING_FRAME_PAYLOAD_LENGTH; import static io.netty.handler.codec.http2.Http2CodecUtil.connectionPrefaceBuf; import static io.netty.handler.codec.http2.Http2CodecUtil.getEmbeddedHttp2Exception; import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR; @@ -96,8 +97,6 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http if (encoder.connection() != decoder.connection()) { throw new IllegalArgumentException("Encoder and Decoder do not share the same connection object"); } - - clientPrefaceString = clientPrefaceString(encoder.connection()); } public Http2Connection connection() { @@ -156,14 +155,21 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http @Override public void handlerAdded(ChannelHandlerContext ctx) throws Exception { + clientPrefaceString = clientPrefaceString(encoder.connection()); // This handler was just added to the context. In case it was handled after // the connection became active, send the connection preface now. sendPreface(ctx); } + /** + * Releases the {@code clientPrefaceString}. Any active streams will be left in the open. + */ @Override protected void handlerRemoved0(ChannelHandlerContext ctx) throws Exception { - dispose(); + if (clientPrefaceString != null) { + clientPrefaceString.release(); + clientPrefaceString = null; + } } @Override @@ -187,10 +193,18 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http @Override public void channelInactive(ChannelHandlerContext ctx) throws Exception { - ChannelFuture future = ctx.newSucceededFuture(); - final Collection streams = connection().activeStreams(); - for (Http2Stream s : streams.toArray(new Http2Stream[streams.size()])) { - closeStream(s, future); + try { + ChannelFuture future = ctx.newSucceededFuture(); + final Collection streams = connection().activeStreams(); + for (Http2Stream s : streams.toArray(new Http2Stream[streams.size()])) { + closeStream(s, future); + } + } finally { + try { + encoder().close(); + } finally { + decoder().close(); + } } super.channelInactive(ctx); } @@ -411,18 +425,6 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http ChannelFutureListener.CLOSE_ON_FAILURE); } - /** - * Disposes of all resources. - */ - private void dispose() { - encoder.close(); - decoder.close(); - if (clientPrefaceString != null) { - clientPrefaceString.release(); - clientPrefaceString = null; - } - } - /** * Decodes the client connection preface string from the input buffer. * diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java index ea9844451c..6c0ebd6037 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java @@ -117,22 +117,27 @@ public class Http2ConnectionHandlerTest { when(ctx.newSucceededFuture()).thenReturn(future); when(ctx.newPromise()).thenReturn(promise); when(ctx.write(any())).thenReturn(future); - - handler = newHandler(); } - private Http2ConnectionHandler newHandler() { - return new Http2ConnectionHandler(decoderBuilder, encoderBuilder); + private Http2ConnectionHandler newHandler() throws Exception { + Http2ConnectionHandler handler = new Http2ConnectionHandler(decoderBuilder, encoderBuilder); + handler.handlerAdded(ctx); + return handler; } @After public void tearDown() throws Exception { - handler.handlerRemoved(ctx); + if (handler != null) { + handler.handlerRemoved(ctx); + } } @Test public void clientShouldSendClientPrefaceStringWhenActive() throws Exception { when(connection.isServer()).thenReturn(false); + when(channel.isActive()).thenReturn(false); + handler = newHandler(); + when(channel.isActive()).thenReturn(true); handler.channelActive(ctx); verify(ctx).write(eq(connectionPrefaceBuf())); } @@ -140,6 +145,9 @@ public class Http2ConnectionHandlerTest { @Test public void serverShouldNotSendClientPrefaceStringWhenActive() throws Exception { when(connection.isServer()).thenReturn(true); + when(channel.isActive()).thenReturn(false); + handler = newHandler(); + when(channel.isActive()).thenReturn(true); handler.channelActive(ctx); verify(ctx, never()).write(eq(connectionPrefaceBuf())); } @@ -158,18 +166,21 @@ public class Http2ConnectionHandlerTest { @Test public void serverReceivingValidClientPrefaceStringShouldContinueReadingFrames() throws Exception { when(connection.isServer()).thenReturn(true); + handler = newHandler(); handler.channelRead(ctx, connectionPrefaceBuf()); verify(decoder).decodeFrame(eq(ctx), any(ByteBuf.class), Matchers.>any()); } @Test public void channelInactiveShouldCloseStreams() throws Exception { + handler = newHandler(); handler.channelInactive(ctx); verify(stream).close(); } @Test public void connectionErrorShouldStartShutdown() throws Exception { + handler = newHandler(); Http2Exception e = new Http2Exception(PROTOCOL_ERROR); when(remote.lastStreamCreated()).thenReturn(STREAM_ID); handler.exceptionCaught(ctx, e); @@ -178,4 +189,14 @@ public class Http2ConnectionHandlerTest { captor.capture(), eq(promise)); captor.getValue().release(); } + + @Test + public void encoderAndDecoderAreClosedOnChannelInactive() throws Exception { + handler = newHandler(); + handler.channelActive(ctx); + when(channel.isActive()).thenReturn(false); + handler.channelInactive(ctx); + verify(encoder).close(); + verify(decoder).close(); + } }