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/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;
@@ -99,8 +100,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() {
@@ -159,14 +158,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
@@ -226,10 +232,18 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
 
     @Override
     public void channelInactive(ChannelHandlerContext ctx) throws Exception {
-        ChannelFuture future = ctx.newSucceededFuture();
-        final Collection<Http2Stream> streams = connection().activeStreams();
-        for (Http2Stream s : streams.toArray(new Http2Stream[streams.size()])) {
-            closeStream(s, future);
+        try {
+            ChannelFuture future = ctx.newSucceededFuture();
+            final Collection<Http2Stream> 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);
     }
@@ -450,18 +464,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.
      *

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.<List<Object>>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();
+    }
 }