Repository: netty/netty
Issue: WebSocketClientProtocolHandshakeHandler leaks
Labels: defect

The new unit test in #2970 did not found  the issue it intended to check but therefore it still detected that there's a leak somewhere. It seems to be in the WebSocketClientProtocolHandshakeHandler which does never release the received HTTP handshake response. The log which shows the leak is attached below:

```
05:39:11.993 [main] ERROR io.netty.util.ResourceLeakDetector - LEAK: ByteBuf.release() was not called before it's garbage-collected.
Recent access records: 1
#1:
    Hint: 'io.netty.handler.codec.http.websocketx.WebSocketClientProtocolHandshakeHandler' will handle the message from this point.
    io.netty.buffer.CompositeByteBuf.touch(CompositeByteBuf.java:1580)
    io.netty.buffer.CompositeByteBuf.touch(CompositeByteBuf.java:40)
    io.netty.buffer.DefaultByteBufHolder.touch(DefaultByteBufHolder.java:79)
    io.netty.handler.codec.http.HttpObjectAggregator$AggregatedFullHttpMessage.touch(HttpObjectAggregator.java:243)
    io.netty.handler.codec.http.HttpObjectAggregator$AggregatedFullHttpResponse.touch(HttpObjectAggregator.java:444)
    io.netty.handler.codec.http.HttpObjectAggregator$AggregatedFullHttpResponse.touch(HttpObjectAggregator.java:363)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:389)
    io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
    io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    io.netty.channel.embedded.EmbeddedEventLoop.invokeChannelRead(EmbeddedEventLoop.java:157)
    io.netty.channel.PausableChannelEventExecutor.invokeChannelRead(PausableChannelEventExecutor.java:86)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:390)
    io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:162)
    io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    io.netty.channel.embedded.EmbeddedEventLoop.invokeChannelRead(EmbeddedEventLoop.java:157)
    io.netty.channel.PausableChannelEventExecutor.invokeChannelRead(PausableChannelEventExecutor.java:86)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:390)
    io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:896)
    io.netty.channel.embedded.EmbeddedChannel.writeInbound(EmbeddedChannel.java:176)
    io.netty.handler.codec.http.websocketx.WebSocketHandshakeHandOverTest.transferAllWithMerge(WebSocketHandshakeHandOverTest.java:115)
    io.netty.handler.codec.http.websocketx.WebSocketHandshakeHandOverTest.testHandover(WebSocketHandshakeHandOverTest.java:86)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:606)
    org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:264)
    org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
    org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:124)
    org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:200)
    org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:153)
    org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)
Created at:
    io.netty.buffer.CompositeByteBuf.<init>(CompositeByteBuf.java:59)
    io.netty.buffer.Unpooled.compositeBuffer(Unpooled.java:355)
    io.netty.handler.codec.MessageAggregator.decode(MessageAggregator.java:241)
    io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:89)
    io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    io.netty.channel.embedded.EmbeddedEventLoop.invokeChannelRead(EmbeddedEventLoop.java:157)
    io.netty.channel.PausableChannelEventExecutor.invokeChannelRead(PausableChannelEventExecutor.java:86)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:390)
    io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:162)
    io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    io.netty.channel.embedded.EmbeddedEventLoop.invokeChannelRead(EmbeddedEventLoop.java:157)
    io.netty.channel.PausableChannelEventExecutor.invokeChannelRead(PausableChannelEventExecutor.java:86)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:390)
    io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:896)
    io.netty.channel.embedded.EmbeddedChannel.writeInbound(EmbeddedChannel.java:176)
    io.netty.handler.codec.http.websocketx.WebSocketHandshakeHandOverTest.transferAllWithMerge(WebSocketHandshakeHandOverTest.java:115)
    io.netty.handler.codec.http.websocketx.WebSocketHandshakeHandOverTest.testHandover(WebSocketHandshakeHandOverTest.java:86)
    sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.lang.reflect.Method.invoke(Method.java:606)
    org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:264)
    org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
    org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:124)
    org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:200)
    org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:153)
    org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)
```


Issue comments:
Fix is provided in #2972 


Pull request review comments:
Nit: Consider saving a reference to the `(FullHttpResponse) msg` here to avoid casting multiple times.

done


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandshakeHandler.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandshakeHandler.java
index f9b8b40c314..bc832510819 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandshakeHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientProtocolHandshakeHandler.java
@@ -51,13 +51,18 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
             return;
         }
 
-        if (!handshaker.isHandshakeComplete()) {
-            handshaker.finishHandshake(ctx.channel(), (FullHttpResponse) msg);
-            ctx.fireUserEventTriggered(
-                    WebSocketClientProtocolHandler.ClientHandshakeStateEvent.HANDSHAKE_COMPLETE);
-            ctx.pipeline().remove(this);
-            return;
+        FullHttpResponse response = (FullHttpResponse) msg;
+        try {
+            if (!handshaker.isHandshakeComplete()) {
+                handshaker.finishHandshake(ctx.channel(), response);
+                ctx.fireUserEventTriggered(
+                        WebSocketClientProtocolHandler.ClientHandshakeStateEvent.HANDSHAKE_COMPLETE);
+                ctx.pipeline().remove(this);
+                return;
+            }
+            throw new IllegalStateException("WebSocketClientHandshaker should have been non finished yet");
+        } finally {
+            response.release();
         }
-        throw new IllegalStateException("WebSocketClientHandshaker should have been non finished yet");
     }
 }


Test patch: