Repository: netty/netty
Issue: @Sharable CombinedChannelDuplexHandler usage problems
Labels: defect

Netty version: 4.1.8.Final

@Sharable CombinedChannelDuplexHandler implementation shares ChannelHandlerContext among different connections. Problem could be reproduced with [this code](https://gist.github.com/errandir/af45f3559421e3d12ace556af3e4999b) _(compile and run with netty-all-4.1.8.Final.jar in classpath)_. Output will differ from time to time.

For example I get:
<pre>
io.netty.channel.CombinedChannelDuplexHandler$1@3e234509: [1-1]
io.netty.channel.CombinedChannelDuplexHandler$1@3e328346: [1-2]
io.netty.channel.CombinedChannelDuplexHandler$1@3e328346: [2-1][2-2][2-3]
io.netty.channel.CombinedChannelDuplexHandler$1@3e328346: [2-4]
io.netty.channel.CombinedChannelDuplexHandler$1@3e328346: [1-3][1-4]
</pre>
but expect that `[1-*]` strings will be handled by the first handler (`3e234509`).

Issue comments:
@errandir `CombinedChannelDuplexHandler` implementation can't be `@Sharable` as it contains state. Let me ensure we throw an exception if someone tries to make a sub-class of `CombinedChannelDuplexHandler ` `@Sharable`.

Thanks for reporting
@normanmaurer There is real lack of such exception and even `CombinedChannelDuplexHandler` comment.
@errandir yep... will have a PR soon. 
Fixed by #6352

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/ByteToMessageCodec.java b/codec/src/main/java/io/netty/handler/codec/ByteToMessageCodec.java
index c7c61f7cef2..7dd80f5d433 100644
--- a/codec/src/main/java/io/netty/handler/codec/ByteToMessageCodec.java
+++ b/codec/src/main/java/io/netty/handler/codec/ByteToMessageCodec.java
@@ -70,7 +70,7 @@ protected ByteToMessageCodec(Class<? extends I> outboundMessageType) {
      *                              {@link ByteBuf}, which is backed by an byte array.
      */
     protected ByteToMessageCodec(boolean preferDirect) {
-        CodecUtil.ensureNotSharable(this);
+        ensureNotSharable();
         outboundMsgMatcher = TypeParameterMatcher.find(this, ByteToMessageCodec.class, "I");
         encoder = new Encoder(preferDirect);
     }
@@ -84,7 +84,7 @@ protected ByteToMessageCodec(boolean preferDirect) {
      *                              {@link ByteBuf}, which is backed by an byte array.
      */
     protected ByteToMessageCodec(Class<? extends I> outboundMessageType, boolean preferDirect) {
-        CodecUtil.ensureNotSharable(this);
+        ensureNotSharable();
         outboundMsgMatcher = TypeParameterMatcher.get(outboundMessageType);
         encoder = new Encoder(preferDirect);
     }
diff --git a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
index a4ccb5aee4d..0c8a78365c0 100644
--- a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
@@ -138,7 +138,7 @@ public ByteBuf cumulate(ByteBufAllocator alloc, ByteBuf cumulation, ByteBuf in)
     private int numReads;
 
     protected ByteToMessageDecoder() {
-        CodecUtil.ensureNotSharable(this);
+        ensureNotSharable();
     }
 
     /**
diff --git a/codec/src/main/java/io/netty/handler/codec/CodecUtil.java b/codec/src/main/java/io/netty/handler/codec/CodecUtil.java
deleted file mode 100644
index 8526decba17..00000000000
--- a/codec/src/main/java/io/netty/handler/codec/CodecUtil.java
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * Copyright 2014 The Netty Project
- *
- * The Netty Project licenses this file to you under the Apache License,
- * version 2.0 (the "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at:
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations
- * under the License.
- */
-package io.netty.handler.codec;
-
-import io.netty.channel.ChannelHandlerAdapter;
-
-final class CodecUtil {
-
-    /**
-     * Throws {@link IllegalStateException} if {@link ChannelHandlerAdapter#isSharable()} returns {@code true}
-     */
-    static void ensureNotSharable(ChannelHandlerAdapter handler) {
-        if (handler.isSharable()) {
-            throw new IllegalStateException("@Sharable annotation is not allowed");
-        }
-    }
-
-    private CodecUtil() { }
-}
diff --git a/transport/src/main/java/io/netty/channel/ChannelHandlerAdapter.java b/transport/src/main/java/io/netty/channel/ChannelHandlerAdapter.java
index 1aaf77b6c33..aadc691d669 100644
--- a/transport/src/main/java/io/netty/channel/ChannelHandlerAdapter.java
+++ b/transport/src/main/java/io/netty/channel/ChannelHandlerAdapter.java
@@ -28,6 +28,15 @@ public abstract class ChannelHandlerAdapter implements ChannelHandler {
     // Not using volatile because it's used only for a sanity check.
     boolean added;
 
+    /**
+     * Throws {@link IllegalStateException} if {@link ChannelHandlerAdapter#isSharable()} returns {@code true}
+     */
+    protected void ensureNotSharable() {
+        if (isSharable()) {
+            throw new IllegalStateException("ChannelHandler " + getClass().getName() + " is not allowed to be shared");
+        }
+    }
+
     /**
      * Return {@code true} if the implementation is {@link Sharable} and so can be added
      * to different {@link ChannelPipeline}s.
diff --git a/transport/src/main/java/io/netty/channel/CombinedChannelDuplexHandler.java b/transport/src/main/java/io/netty/channel/CombinedChannelDuplexHandler.java
index c1f156080a2..5cb52668d7b 100644
--- a/transport/src/main/java/io/netty/channel/CombinedChannelDuplexHandler.java
+++ b/transport/src/main/java/io/netty/channel/CombinedChannelDuplexHandler.java
@@ -45,12 +45,15 @@ public class CombinedChannelDuplexHandler<I extends ChannelInboundHandler, O ext
      * {@link #init(ChannelInboundHandler, ChannelOutboundHandler)} before adding this handler into a
      * {@link ChannelPipeline}.
      */
-    protected CombinedChannelDuplexHandler() { }
+    protected CombinedChannelDuplexHandler() {
+        ensureNotSharable();
+    }
 
     /**
      * Creates a new instance that combines the specified two handlers into one.
      */
     public CombinedChannelDuplexHandler(I inboundHandler, O outboundHandler) {
+        ensureNotSharable();
         init(inboundHandler, outboundHandler);
     }
 


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/CombinedChannelDuplexHandlerTest.java b/transport/src/test/java/io/netty/channel/CombinedChannelDuplexHandlerTest.java
index 27f9b8d11c2..f697e0b50f4 100644
--- a/transport/src/test/java/io/netty/channel/CombinedChannelDuplexHandlerTest.java
+++ b/transport/src/test/java/io/netty/channel/CombinedChannelDuplexHandlerTest.java
@@ -387,4 +387,14 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
         promise.syncUninterruptibly();
         ch.finish();
     }
+
+    @Test(expected = IllegalStateException.class)
+    public void testNotSharable() {
+        new CombinedChannelDuplexHandler<ChannelInboundHandler, ChannelOutboundHandler>() {
+            @Override
+            public boolean isSharable() {
+                return true;
+            }
+        };
+    }
 }