Repository: netty/netty
Issue: ClosedChannelException in doRegister() of LocalChannel
Labels: defect

Hi,
we try to move a LocalChannel for one EventLoopGroup to another. Therefore, we use the following (simplyfied) code. This piece of code is called from a thread of the EventLoopGroup, which is assigned to the LocalChannel by now. The auto read function of the channel is disabled. We are using Netty 4.0.18.Final. Do you need any further information?

```
channel.deregister().addListener(new ChannelFutureListener() {
   @Override
   public void operationComplete(ChannelFuture future) throws Exception {
      try {
        Channel channel = future.channel();
        EventLoopGroup eventLoopGroup = 
        if (channel instanceof LocalChannel) {
           eventLoopGroup = Helper.localInputEventLoopGroup;
        } else {
           eventLoopGroup = Helper.networkInputEventLoopGroup;
        }

         // Change event loop group.
         eventLoopGroup.register(channel).sync();

         // Enable auto read again.
         channel.config().setAutoRead(true);
      } catch (Throwable t) {
        LOG.error(t.getLocalizedMessage(), t);
      }
   }
}).sync();
```

We always get a ClosedChannelException when the register()-method is called. The reason for this is that the LocalChannel (and its peer) is closed in the deregister()-method. Is there any clean way to move a LocalChannel to another EventLoopGroup without closing it and its peer? (without applying the very dirty reflection hack we're using by now...)


Issue comments:
@Teots can I see your "dirty reflection hack" ;) ?

Sure you can. Here we go:

```
// channel is passed into the function and is of type Channel
if (channel instanceof LocalChannel) {
   Class<?> clazz = channel.getClass();
   Field stateField = clazz.getDeclaredField("state");
   stateField.setAccessible(true);
   stateField.setInt(channel, 3);
}

channel.deregister().addListener(new ChannelFutureListener() {
   @Override
   public void operationComplete(ChannelFuture future) throws Exception {
      try {
        Channel channel = future.channel();
        EventLoopGroup eventLoopGroup = 
        if (channel instanceof LocalChannel) {
           eventLoopGroup = Helper.localInputEventLoopGroup;

           Class<?> clazz = channel.getClass();
           Field stateField = clazz.getDeclaredField("state");
           stateField.setAccessible(true);
           stateField.setInt(channel, 0);

           Field peerField = clazz.getDeclaredField("peer");
           peerField.setAccessible(true);
           LocalChannel peer = (LocalChannel) peerField.get(channel);

           Field connectPromiseField = clazz.getDeclaredField("connectPromise");
           connectPromiseField.setAccessible(true);
           connectPromiseField.set(peer, peer.unsafe().voidPromise());

           // Change event loop group.
           eventLoopGroup.register(channel).sync();
        } else {
           eventLoopGroup = Helper.networkInputEventLoopGroup;

           // Change event loop group.
           eventLoopGroup.register(channel).sync();
        }

         // Enable auto read again.
         channel.config().setAutoRead(true);
      } catch (Throwable t) {
        LOG.error(t.getLocalizedMessage(), t);
      }
   }
}).sync();
```

@Teots could you please check if the PR #2413 fix the issue for you ?


Pull request review comments:
'Check if both peer and parent are non-null because this channel was created by a ..'

I would always prefer 'because' to 'as' because it's less confusing to a reader.


Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/local/LocalChannel.java b/transport/src/main/java/io/netty/channel/local/LocalChannel.java
index 24eb2d78510..3aba344be48 100644
--- a/transport/src/main/java/io/netty/channel/local/LocalChannel.java
+++ b/transport/src/main/java/io/netty/channel/local/LocalChannel.java
@@ -153,7 +153,12 @@ protected SocketAddress remoteAddress0() {
 
     @Override
     protected void doRegister() throws Exception {
-        if (peer != null) {
+        // Check if both peer and parent are non-null because this channel was created by a LocalServerChannel.
+        // This is needed as a peer may not be null also if a LocalChannel was connected before and
+        // deregistered / registered later again.
+        //
+        // See https://github.com/netty/netty/issues/2400
+        if (peer != null && parent() != null) {
             // Store the peer in a local variable as it may be set to null if doClose() is called.
             // Because of this we also set registerInProgress to true as we check for this in doClose() and make sure
             // we delay the fireChannelInactive() to be fired after the fireChannelActive() and so keep the correct
@@ -235,9 +240,7 @@ public void run() {
 
     @Override
     protected void doDeregister() throws Exception {
-        if (isOpen()) {
-            unsafe().close(unsafe().voidPromise());
-        }
+        // Just remove the shutdownHook as this Channel may be closed later or registered to another EventLoop
         ((SingleThreadEventExecutor) eventLoop()).removeShutdownHook(shutdownHook);
     }
 


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/local/LocalChannelTest.java b/transport/src/test/java/io/netty/channel/local/LocalChannelTest.java
index adcec5f99d7..d72d9d31974 100644
--- a/transport/src/test/java/io/netty/channel/local/LocalChannelTest.java
+++ b/transport/src/test/java/io/netty/channel/local/LocalChannelTest.java
@@ -29,7 +29,6 @@
 import io.netty.util.concurrent.EventExecutor;
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
-import org.junit.Assert;
 import org.junit.Test;
 
 import java.nio.channels.ClosedChannelException;
@@ -248,6 +247,39 @@ protected void initChannel(Channel ch) throws Exception {
         }
     }
 
+    @Test
+    public void testReRegister() {
+        EventLoopGroup group1 = new LocalEventLoopGroup();
+        EventLoopGroup group2 = new LocalEventLoopGroup();
+        LocalAddress addr = new LocalAddress(LOCAL_ADDR_ID);
+        Bootstrap cb = new Bootstrap();
+        ServerBootstrap sb = new ServerBootstrap();
+
+        cb.group(group1)
+                .channel(LocalChannel.class)
+                .handler(new TestHandler());
+
+        sb.group(group2)
+                .channel(LocalServerChannel.class)
+                .childHandler(new ChannelInitializer<LocalChannel>() {
+                    @Override
+                    public void initChannel(LocalChannel ch) throws Exception {
+                        ch.pipeline().addLast(new TestHandler());
+                    }
+                });
+
+        // Start server
+        final Channel sc = sb.bind(addr).syncUninterruptibly().channel();
+
+        // Connect to the server
+        final Channel cc = cb.connect(addr).syncUninterruptibly().channel();
+
+        cc.deregister().syncUninterruptibly();
+        // Change event loop group.
+        group2.register(cc).syncUninterruptibly();
+        cc.close().syncUninterruptibly();
+        sc.close().syncUninterruptibly();
+    }
     static class TestHandler extends ChannelInboundHandlerAdapter {
         @Override
         public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {