Repository: netty/netty
Issue: not sure about SingleThreadEventLoop.register(Channel, ChannelPromise)
Labels: defect

hi all, 
a little confused that why not  this method change to register(ChannelPromise) since the channel is a param in DefaultChannelPromise's constructor...(will the promise change channel? ) ,not sure, thanks
source code:
    @Override
    public ChannelFuture register(Channel channel) {
        return register(channel, new DefaultChannelPromise(channel, this));
    }
    @Override
    public ChannelFuture register(final Channel channel, final ChannelPromise promise) {


Issue comments:
That's a good point.  If a user specifies a promise when registering, a user doesn't really need to specify a channel because an event loop can get the channel from the specified promise.  Let me fix this in 4.1.

@colalife @trustin makes sense

really encourage me，btw，i see it implements from EventLoopGroup，some classes have it

Are you suggesting that `EventLoop` extending `EventLoopGroup` is a problem, or..?

I think the reasonable fix for this issue is to remove `register(Channel, ChannelPromise)` (with proper deprecation,) and let people always use `register(Channel)`.  What do you think?

@trustin  +1

yeah, i agree, and of course other classes et. MultithreadEventLoopGroup... who implement it

@trustin I guess we can only do this for 5.0.0 as it will break the api

@trustin do we want to "fix this" for 4.1.0.Final or not ?

Fixed by https://github.com/netty/netty/pull/5231


Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/EventLoopGroup.java b/transport/src/main/java/io/netty/channel/EventLoopGroup.java
index e2bf7c2c0af..3e390e82230 100644
--- a/transport/src/main/java/io/netty/channel/EventLoopGroup.java
+++ b/transport/src/main/java/io/netty/channel/EventLoopGroup.java
@@ -35,9 +35,18 @@ public interface EventLoopGroup extends EventExecutorGroup {
      */
     ChannelFuture register(Channel channel);
 
+    /**
+     * Register a {@link Channel} with this {@link EventLoop} using a {@link ChannelFuture}. The passed
+     * {@link ChannelFuture} will get notified once the registration was complete and also will get returned.
+     */
+    ChannelFuture register(ChannelPromise promise);
+
     /**
      * Register a {@link Channel} with this {@link EventLoop}. The passed {@link ChannelFuture}
      * will get notified once the registration was complete and also will get returned.
+     *
+     * @deprecated Use {@link #register(ChannelPromise)} instead.
      */
+    @Deprecated
     ChannelFuture register(Channel channel, ChannelPromise promise);
 }
diff --git a/transport/src/main/java/io/netty/channel/MultithreadEventLoopGroup.java b/transport/src/main/java/io/netty/channel/MultithreadEventLoopGroup.java
index aae17b4505e..327bc16dd58 100644
--- a/transport/src/main/java/io/netty/channel/MultithreadEventLoopGroup.java
+++ b/transport/src/main/java/io/netty/channel/MultithreadEventLoopGroup.java
@@ -75,6 +75,12 @@ public ChannelFuture register(Channel channel) {
         return next().register(channel);
     }
 
+    @Override
+    public ChannelFuture register(ChannelPromise promise) {
+        return next().register(promise);
+    }
+
+    @Deprecated
     @Override
     public ChannelFuture register(Channel channel, ChannelPromise promise) {
         return next().register(channel, promise);
diff --git a/transport/src/main/java/io/netty/channel/SingleThreadEventLoop.java b/transport/src/main/java/io/netty/channel/SingleThreadEventLoop.java
index 81cc3dbaefa..c76beb89cae 100644
--- a/transport/src/main/java/io/netty/channel/SingleThreadEventLoop.java
+++ b/transport/src/main/java/io/netty/channel/SingleThreadEventLoop.java
@@ -16,6 +16,7 @@
 package io.netty.channel;
 
 import io.netty.util.concurrent.SingleThreadEventExecutor;
+import io.netty.util.internal.ObjectUtil;
 
 import java.util.concurrent.Executor;
 import java.util.concurrent.ThreadFactory;
@@ -53,9 +54,17 @@ public ChannelHandlerInvoker asInvoker() {
 
     @Override
     public ChannelFuture register(Channel channel) {
-        return register(channel, new DefaultChannelPromise(channel, this));
+        return register(new DefaultChannelPromise(channel, this));
     }
 
+    @Override
+    public ChannelFuture register(final ChannelPromise promise) {
+        ObjectUtil.checkNotNull(promise, "promise");
+        promise.channel().unsafe().register(this, promise);
+        return promise;
+    }
+
+    @Deprecated
     @Override
     public ChannelFuture register(final Channel channel, final ChannelPromise promise) {
         if (channel == null) {
diff --git a/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoop.java b/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoop.java
index c73f36a929c..b10d79de7eb 100644
--- a/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoop.java
+++ b/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoop.java
@@ -30,6 +30,21 @@ public ThreadPerChannelEventLoop(ThreadPerChannelEventLoopGroup parent) {
         this.parent = parent;
     }
 
+    @Override
+    public ChannelFuture register(ChannelPromise promise) {
+        return super.register(promise).addListener(new ChannelFutureListener() {
+            @Override
+            public void operationComplete(ChannelFuture future) throws Exception {
+                if (future.isSuccess()) {
+                    ch = future.channel();
+                } else {
+                    deregister();
+                }
+            }
+        });
+    }
+
+    @Deprecated
     @Override
     public ChannelFuture register(Channel channel, ChannelPromise promise) {
         return super.register(channel, promise).addListener(new ChannelFutureListener() {
diff --git a/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java b/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java
index 700625506cc..94826c5f732 100644
--- a/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java
+++ b/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java
@@ -78,7 +78,7 @@ protected ThreadPerChannelEventLoopGroup() {
      * @param maxChannels       the maximum number of channels to handle with this instance. Once you try to register
      *                          a new {@link Channel} and the maximum is exceed it will throw an
      *                          {@link ChannelException}. on the {@link #register(Channel)} and
-     *                          {@link #register(Channel, ChannelPromise)} method.
+     *                          {@link #register(ChannelPromise)} method.
      *                          Use {@code 0} to use no limit
      */
     protected ThreadPerChannelEventLoopGroup(int maxChannels) {
@@ -91,7 +91,7 @@ protected ThreadPerChannelEventLoopGroup(int maxChannels) {
      * @param maxChannels       the maximum number of channels to handle with this instance. Once you try to register
      *                          a new {@link Channel} and the maximum is exceed it will throw an
      *                          {@link ChannelException} on the {@link #register(Channel)} and
-     *                          {@link #register(Channel, ChannelPromise)} method.
+     *                          {@link #register(ChannelPromise)} method.
      *                          Use {@code 0} to use no limit
      * @param threadFactory     the {@link ThreadFactory} used to create new {@link Thread} instances that handle the
      *                          registered {@link Channel}s
@@ -107,7 +107,7 @@ protected ThreadPerChannelEventLoopGroup(int maxChannels, ThreadFactory threadFa
      * @param maxChannels       the maximum number of channels to handle with this instance. Once you try to register
      *                          a new {@link Channel} and the maximum is exceed it will throw an
      *                          {@link ChannelException} on the {@link #register(Channel)} and
-     *                          {@link #register(Channel, ChannelPromise)} method.
+     *                          {@link #register(ChannelPromise)} method.
      *                          Use {@code 0} to use no limit
      * @param executor          the {@link Executor} used to create new {@link Thread} instances that handle the
      *                          registered {@link Channel}s
@@ -281,12 +281,23 @@ public ChannelFuture register(Channel channel) {
         }
         try {
             EventLoop l = nextChild();
-            return l.register(channel, new DefaultChannelPromise(channel, l));
+            return l.register(new DefaultChannelPromise(channel, l));
         } catch (Throwable t) {
             return new FailedChannelFuture(channel, GlobalEventExecutor.INSTANCE, t);
         }
     }
 
+    @Override
+    public ChannelFuture register(ChannelPromise promise) {
+        try {
+            return nextChild().register(promise);
+        } catch (Throwable t) {
+            promise.setFailure(t);
+            return promise;
+        }
+    }
+
+    @Deprecated
     @Override
     public ChannelFuture register(Channel channel, ChannelPromise promise) {
         if (channel == null) {
diff --git a/transport/src/main/java/io/netty/channel/embedded/EmbeddedEventLoop.java b/transport/src/main/java/io/netty/channel/embedded/EmbeddedEventLoop.java
index f88668cedaf..46aeff25905 100644
--- a/transport/src/main/java/io/netty/channel/embedded/EmbeddedEventLoop.java
+++ b/transport/src/main/java/io/netty/channel/embedded/EmbeddedEventLoop.java
@@ -26,6 +26,7 @@
 import io.netty.channel.EventLoopGroup;
 import io.netty.util.concurrent.AbstractScheduledEventExecutor;
 import io.netty.util.concurrent.Future;
+import io.netty.util.internal.ObjectUtil;
 
 import java.net.SocketAddress;
 import java.util.ArrayDeque;
@@ -126,9 +127,17 @@ public boolean awaitTermination(long timeout, TimeUnit unit) {
 
     @Override
     public ChannelFuture register(Channel channel) {
-        return register(channel, new DefaultChannelPromise(channel, this));
+        return register(new DefaultChannelPromise(channel, this));
     }
 
+    @Override
+    public ChannelFuture register(ChannelPromise promise) {
+        ObjectUtil.checkNotNull(promise, "promise");
+        promise.channel().unsafe().register(this, promise);
+        return promise;
+    }
+
+    @Deprecated
     @Override
     public ChannelFuture register(Channel channel, ChannelPromise promise) {
         channel.unsafe().register(this, promise);
diff --git a/transport/src/main/java/io/netty/channel/oio/OioEventLoopGroup.java b/transport/src/main/java/io/netty/channel/oio/OioEventLoopGroup.java
index e803f8b325e..684af409839 100644
--- a/transport/src/main/java/io/netty/channel/oio/OioEventLoopGroup.java
+++ b/transport/src/main/java/io/netty/channel/oio/OioEventLoopGroup.java
@@ -46,7 +46,7 @@ public OioEventLoopGroup() {
      * @param maxChannels       the maximum number of channels to handle with this instance. Once you try to register
      *                          a new {@link Channel} and the maximum is exceed it will throw an
      *                          {@link ChannelException} on the {@link #register(Channel)} and
-     *                          {@link #register(Channel, ChannelPromise)} method.
+     *                          {@link #register(ChannelPromise)} method.
      *                          Use {@code 0} to use no limit
      */
     public OioEventLoopGroup(int maxChannels) {
@@ -59,7 +59,7 @@ public OioEventLoopGroup(int maxChannels) {
      * @param maxChannels       the maximum number of channels to handle with this instance. Once you try to register
      *                          a new {@link Channel} and the maximum is exceed it will throw an
      *                          {@link ChannelException} on the {@link #register(Channel)} and
-     *                          {@link #register(Channel, ChannelPromise)} method.
+     *                          {@link #register(ChannelPromise)} method.
      *                          Use {@code 0} to use no limit
      * @param executor     the {@link Executor} used to create new {@link Thread} instances that handle the
      *                          registered {@link Channel}s
@@ -74,7 +74,7 @@ public OioEventLoopGroup(int maxChannels, Executor executor) {
      * @param maxChannels       the maximum number of channels to handle with this instance. Once you try to register
      *                          a new {@link Channel} and the maximum is exceed it will throw an
      *                          {@link ChannelException} on the {@link #register(Channel)} and
-     *                          {@link #register(Channel, ChannelPromise)} method.
+     *                          {@link #register(ChannelPromise)} method.
      *                          Use {@code 0} to use no limit
      * @param threadFactory     the {@link ThreadFactory} used to create new {@link Thread} instances that handle the
      *                          registered {@link Channel}s


Test patch:
diff --git a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java
index 53a15004ed8..cba71b3e4a1 100644
--- a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java
+++ b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java
@@ -275,6 +275,11 @@ public ChannelFuture register(Channel channel) {
             return promise;
         }
 
+        @Override
+        public ChannelFuture register(ChannelPromise promise) {
+            throw new UnsupportedOperationException();
+        }
+
         @Override
         public ChannelFuture register(Channel channel, final ChannelPromise promise) {
             throw new UnsupportedOperationException();
diff --git a/transport/src/test/java/io/netty/channel/SingleThreadEventLoopTest.java b/transport/src/test/java/io/netty/channel/SingleThreadEventLoopTest.java
index fd862cd5675..41b53d90c0e 100644
--- a/transport/src/test/java/io/netty/channel/SingleThreadEventLoopTest.java
+++ b/transport/src/test/java/io/netty/channel/SingleThreadEventLoopTest.java
@@ -381,7 +381,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
         }
 
         try {
-            ChannelFuture f = loopA.register(ch, promise);
+            ChannelFuture f = loopA.register(promise);
             f.awaitUninterruptibly();
             assertFalse(f.isSuccess());
             assertThat(f.cause(), is(instanceOf(RejectedExecutionException.class)));
diff --git a/transport/src/test/java/io/netty/channel/ThreadPerChannelEventLoopGroupTest.java b/transport/src/test/java/io/netty/channel/ThreadPerChannelEventLoopGroupTest.java
index 2c936bd2333..cf02b18090c 100644
--- a/transport/src/test/java/io/netty/channel/ThreadPerChannelEventLoopGroupTest.java
+++ b/transport/src/test/java/io/netty/channel/ThreadPerChannelEventLoopGroupTest.java
@@ -82,7 +82,7 @@ private static void runTest(ThreadPerChannelEventLoopGroup loopGroup) throws Int
         ChannelGroup channelGroup = new DefaultChannelGroup(testExecutor);
         while (taskCount-- > 0) {
             Channel channel = new EmbeddedChannel(NOOP_HANDLER);
-            loopGroup.register(channel, new DefaultChannelPromise(channel, testExecutor));
+            loopGroup.register(new DefaultChannelPromise(channel, testExecutor));
             channelGroup.add(channel);
         }
         channelGroup.close().sync();