Repository: netty/netty
Issue: ChannelInitializer does not propagate Exceptions
Labels: defect

Netty version: 5.0.0.Alpha2 (but others as well)

Context:
When an exception occurs in a user defined initChannel, it is not propagated through the framework so that it can be handled with exceptionCaught. Instead, channelRegistered logs a warning and dumps a stack trace.

Steps to reproduce:
Throw a generic exception in an initChannel from the netty examples should do the trick.


Issue comments:
@zoowar the question is how we should propagate here as if initChannel(...) throws an exception chances are good that you will not have an handler in the pipeline that can handle the exception.

@normanmaurer When there is no exceptionCaught implemented by any of the pipeline handlers, the framework handles the exception by logging the following message:

> i.n.channel.DefaultChannelPipeline - An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.

It seems to me that ChannelInitializer/channelRegistered could log the specialized message ""Failed to initialize a channel." and throw the exception, which will be handled by the default handler or the user defined handler.

I'm not a java programmer, so the only question in my mind is how does the context get closed when an exception occurs in ChannelInitializer? Does it get closed in the default exceptionCaught path? If it doesn't, then ChannelInitializer could override exceptionCaught and close the context. Then document that users who wish to override ChannelInitializer/exceptionCaught must close the context.

I like the idea of letting `ChannelInitializer.channelRegistered()` propagate the exception so that `ChannelInitializer.exceptionCaught()` closes the connection by default.  That way, a user can override the default behavior.

Let me fix this

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


Pull request review comments:
Instantiate this exception outside the channel scope so you can do `assertSame` at the end of the test?


Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/ChannelInitializer.java b/transport/src/main/java/io/netty/channel/ChannelInitializer.java
index f346f553cda..5db2c291805 100644
--- a/transport/src/main/java/io/netty/channel/ChannelInitializer.java
+++ b/transport/src/main/java/io/netty/channel/ChannelInitializer.java
@@ -56,29 +56,33 @@ public abstract class ChannelInitializer<C extends Channel> extends ChannelInbou
      * will be removed from the {@link ChannelPipeline} of the {@link Channel}.
      *
      * @param ch            the {@link Channel} which was registered.
-     * @throws Exception    is thrown if an error occurs. In that case the {@link Channel} will be closed.
+     * @throws Exception    is thrown if an error occurs. In that case it will be handled by
+     *                      {@link #exceptionCaught(ChannelHandlerContext, Throwable)} which will by default close
+     *                      the {@link Channel}.
      */
     protected abstract void initChannel(C ch) throws Exception;
 
     @Override
     @SuppressWarnings("unchecked")
     public final void channelRegistered(ChannelHandlerContext ctx) throws Exception {
-        ChannelPipeline pipeline = ctx.pipeline();
-        boolean success = false;
+        initChannel((C) ctx.channel());
+        ctx.pipeline().remove(this);
+        ctx.fireChannelRegistered();
+    }
+
+    /**
+     * Handle the {@link Throwable} by logging and closing the {@link Channel}. Sub-classes may override this.
+     */
+    @Override
+    public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
+        logger.warn("Failed to initialize a channel. Closing: " + ctx.channel(), cause);
         try {
-            initChannel((C) ctx.channel());
-            pipeline.remove(this);
-            ctx.fireChannelRegistered();
-            success = true;
-        } catch (Throwable t) {
-            logger.warn("Failed to initialize a channel. Closing: " + ctx.channel(), t);
-        } finally {
+            ChannelPipeline pipeline = ctx.pipeline();
             if (pipeline.context(this) != null) {
                 pipeline.remove(this);
             }
-            if (!success) {
-                ctx.close();
-            }
+        } finally {
+            ctx.close();
         }
     }
 }


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
index 40749b59db5..11503bd90fe 100644
--- a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
+++ b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
@@ -21,6 +21,7 @@
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelHandler.Sharable;
+import io.netty.channel.embedded.EmbeddedChannel;
 import io.netty.channel.local.LocalAddress;
 import io.netty.channel.local.LocalChannel;
 import io.netty.channel.local.LocalEventLoopGroup;
@@ -535,6 +536,29 @@ public void testLastHandlerEmptyPipeline() throws Exception {
         assertNull(pipeline.last());
     }
 
+    @Test(timeout = 5000)
+    public void testChannelInitializerException() throws Exception {
+        final IllegalStateException exception = new IllegalStateException();
+        final AtomicReference<Throwable> error = new AtomicReference<Throwable>();
+        final CountDownLatch latch = new CountDownLatch(1);
+        EmbeddedChannel channel = new EmbeddedChannel(new ChannelInitializer<Channel>() {
+            @Override
+            protected void initChannel(Channel ch) throws Exception {
+                throw exception;
+            }
+
+            @Override
+            public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
+                super.exceptionCaught(ctx, cause);
+                error.set(cause);
+                latch.countDown();
+            }
+        });
+        latch.await();
+        assertFalse(channel.isActive());
+        assertSame(exception, error.get());
+    }
+
     private static int next(AbstractChannelHandlerContext ctx) {
         AbstractChannelHandlerContext next = ctx.next;
         if (next == null) {