Repository: netty/netty
Issue: IllegalReferenceCountException from HttpObjectDecoder when switching a protocol
Labels: defect

I found `HttpObjectDecoder.decode()` raises an `IllegalReferenceCountException` at the `case UPGRADED` block.  After pulling my hairs, I found that it can happen when a user removes the `HttpObjectDecoder` from the pipeline via `HttpClientUpgradeHandler`.  The following buffer access trace should be a good hint:

```
Recent access records: 5
#5:
    io.netty.buffer.AdvancedLeakAwareByteBuf.readBytes(AdvancedLeakAwareByteBuf.java:434)
    io.netty.handler.codec.ByteToMessageDecoder.handlerRemoved(ByteToMessageDecoder.java:214)
    io.netty.channel.DefaultChannelPipeline.callHandlerRemoved0(DefaultChannelPipeline.java:629)
    io.netty.channel.DefaultChannelPipeline.callHandlerRemoved(DefaultChannelPipeline.java:623)
    io.netty.channel.DefaultChannelPipeline.remove0(DefaultChannelPipeline.java:452)
    io.netty.channel.DefaultChannelPipeline.remove(DefaultChannelPipeline.java:423)
    io.netty.channel.DefaultChannelPipeline.remove(DefaultChannelPipeline.java:412)
    io.netty.handler.codec.http.HttpClientCodec.upgradeFrom(HttpClientCodec.java:95)
    io.netty.handler.codec.http.HttpClientUpgradeHandler.decode(HttpClientUpgradeHandler.java:230)
    io.netty.handler.codec.http.HttpClientUpgradeHandler.decode(HttpClientUpgradeHandler.java:38)
    io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:89)
    io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    io.netty.channel.DefaultChannelHandlerInvoker.invokeChannelRead(DefaultChannelHandlerInvoker.java:163)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:155)
    io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:276)
    io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:354)
    io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:244)
    io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    io.netty.channel.DefaultChannelHandlerInvoker.invokeChannelRead(DefaultChannelHandlerInvoker.java:163)
    io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:155)
    io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:950)
    io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:125)
    io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:510)
    io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:467)
    io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:381)
    io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:353)
    io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:742)
    io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:137)
    java.lang.Thread.run(Thread.java:745)
```

It basically means:
1. HttpObjectDecoder decodes the upgrade response from the server. The HttpObjectDecoder enters the UPGRADED state
2. HttpClientUpgradeHandler upgrades the protocol and removes the HttpObjectDecoder from the pipeline. HttpObjectDecoder.handlerRemoved() releases the buffer, but we are still at HttpObjectDecoder.decode().
3. A user gets an IllegalReferenceCountException.


Issue comments:
I guess the root cause of this issue is at `HttpClientCodec.upgradeFrom()`, which removes the `Decoder` immediately. Perhaps we should remove it using `ctx.executor().execute()`?

To add a bit more, I'm using 101 to upgrade from HTTP/1 to HTTP/2. I sometimes get an `IllegalReferenceCountException`, a `First received frame was not SETTINGS.` error, or both. After removing the decoder using `ctx.executor().execute()`, the problem(s) went away.

The 'first received frame was not SETTINGS' error seems to be because the `channelRead` event that contains the first `SETTINGS` frame is produced _before_ `Http2ConnectionHandler` is added and thus lost at the end of the pipeline.

@trustin sounds like a valid fix. 

sounds reasonable to me too

@trustin sgtm

@trustin will you submit a PR for this ?


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
index cae9055b29c..c19da6fec48 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
@@ -19,7 +19,9 @@
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandlerAppender;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPipeline;
 import io.netty.handler.codec.PrematureChannelClosureException;
+import io.netty.util.internal.OneTimeTask;
 
 import java.util.ArrayDeque;
 import java.util.List;
@@ -92,8 +94,15 @@ public HttpClientCodec(
      */
     @Override
     public void upgradeFrom(ChannelHandlerContext ctx) {
-        ctx.pipeline().remove(Decoder.class);
-        ctx.pipeline().remove(Encoder.class);
+        final ChannelPipeline p = ctx.pipeline();
+        // Remove the decoder later so that the decoder can enter the 'UPGRADED' state and forward the remaining data.
+        ctx.executor().execute(new OneTimeTask() {
+            @Override
+            public void run() {
+                p.remove(decoder());
+            }
+        });
+        p.remove(encoder());
     }
 
     /**


Test patch: