Repository: netty/netty
Issue: Provide more information with the ClosedChannelException when fail operation with it
Labels: improvement

If we close a Channel because of a failed write we should store the "original" exception in the `ClosedChannelException` that will be used to fail further operations to make it easier to understand why the Channel was closed before.

This came up yesterday in a discussion with @Scottmitch @ejona86 @carl-mastrangelo .

Issue comments:
+1

Pull request review comments:
this will spuriously succeed on some machines won't it?
It does not matter what we pass here as I implement `AbstractUnsafe.connect(...)` which will just notify the promise. Remember this is no socket etc. 

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/AbstractChannel.java b/transport/src/main/java/io/netty/channel/AbstractChannel.java
index 11c14a9624b..8f8fe2f787a 100644
--- a/transport/src/main/java/io/netty/channel/AbstractChannel.java
+++ b/transport/src/main/java/io/netty/channel/AbstractChannel.java
@@ -44,14 +44,14 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha
 
     private static final InternalLogger logger = InternalLoggerFactory.getInstance(AbstractChannel.class);
 
-    private static final ClosedChannelException FLUSH0_CLOSED_CHANNEL_EXCEPTION = ThrowableUtil.unknownStackTrace(
-            new ClosedChannelException(), AbstractUnsafe.class, "flush0()");
     private static final ClosedChannelException ENSURE_OPEN_CLOSED_CHANNEL_EXCEPTION = ThrowableUtil.unknownStackTrace(
-            new ClosedChannelException(), AbstractUnsafe.class, "ensureOpen(...)");
+            new ExtendedClosedChannelException(null), AbstractUnsafe.class, "ensureOpen(...)");
     private static final ClosedChannelException CLOSE_CLOSED_CHANNEL_EXCEPTION = ThrowableUtil.unknownStackTrace(
             new ClosedChannelException(), AbstractUnsafe.class, "close(...)");
     private static final ClosedChannelException WRITE_CLOSED_CHANNEL_EXCEPTION = ThrowableUtil.unknownStackTrace(
-            new ClosedChannelException(), AbstractUnsafe.class, "write(...)");
+            new ExtendedClosedChannelException(null), AbstractUnsafe.class, "write(...)");
+    private static final ClosedChannelException FLUSH0_CLOSED_CHANNEL_EXCEPTION = ThrowableUtil.unknownStackTrace(
+            new ExtendedClosedChannelException(null), AbstractUnsafe.class, "flush0()");
     private static final NotYetConnectedException FLUSH0_NOT_YET_CONNECTED_EXCEPTION = ThrowableUtil.unknownStackTrace(
             new NotYetConnectedException(), AbstractUnsafe.class, "flush0()");
 
@@ -67,6 +67,7 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha
     private volatile EventLoop eventLoop;
     private volatile boolean registered;
     private boolean closeInitiated;
+    private Throwable initialCloseCause;
 
     /** Cache for the string representation of this channel */
     private boolean strValActive;
@@ -870,7 +871,7 @@ public final void write(Object msg, ChannelPromise promise) {
                 // need to fail the future right away. If it is not null the handling of the rest
                 // will be done in flush0()
                 // See https://github.com/netty/netty/issues/2362
-                safeSetFailure(promise, WRITE_CLOSED_CHANNEL_EXCEPTION);
+                safeSetFailure(promise, newWriteException(initialCloseCause));
                 // release message now to prevent resource-leak
                 ReferenceCountUtil.release(msg);
                 return;
@@ -926,7 +927,7 @@ protected void flush0() {
                         outboundBuffer.failFlushed(FLUSH0_NOT_YET_CONNECTED_EXCEPTION, true);
                     } else {
                         // Do not trigger channelWritabilityChanged because the channel is closed already.
-                        outboundBuffer.failFlushed(FLUSH0_CLOSED_CHANNEL_EXCEPTION, false);
+                        outboundBuffer.failFlushed(newFlush0Exception(initialCloseCause), false);
                     }
                 } finally {
                     inFlush0 = false;
@@ -946,12 +947,14 @@ protected void flush0() {
                      * This is needed as otherwise {@link #isActive()} , {@link #isOpen()} and {@link #isWritable()}
                      * may still return {@code true} even if the channel should be closed as result of the exception.
                      */
-                    close(voidPromise(), t, FLUSH0_CLOSED_CHANNEL_EXCEPTION, false);
+                    initialCloseCause = t;
+                    close(voidPromise(), t, newFlush0Exception(t), false);
                 } else {
                     try {
                         shutdownOutput(voidPromise(), t);
                     } catch (Throwable t2) {
-                        close(voidPromise(), t2, FLUSH0_CLOSED_CHANNEL_EXCEPTION, false);
+                        initialCloseCause = t;
+                        close(voidPromise(), t2, newFlush0Exception(t), false);
                     }
                 }
             } finally {
@@ -959,6 +962,30 @@ protected void flush0() {
             }
         }
 
+        private ClosedChannelException newWriteException(Throwable cause) {
+            if (cause == null) {
+                return WRITE_CLOSED_CHANNEL_EXCEPTION;
+            }
+            return ThrowableUtil.unknownStackTrace(
+                    new ExtendedClosedChannelException(cause), AbstractUnsafe.class, "write(...)");
+        }
+
+        private ClosedChannelException newFlush0Exception(Throwable cause) {
+            if (cause == null) {
+                return FLUSH0_CLOSED_CHANNEL_EXCEPTION;
+            }
+            return ThrowableUtil.unknownStackTrace(
+                    new ExtendedClosedChannelException(cause), AbstractUnsafe.class, "flush0()");
+        }
+
+        private ClosedChannelException newEnsureOpenException(Throwable cause) {
+            if (cause == null) {
+                return ENSURE_OPEN_CLOSED_CHANNEL_EXCEPTION;
+            }
+            return ThrowableUtil.unknownStackTrace(
+                    new ExtendedClosedChannelException(cause), AbstractUnsafe.class, "ensureOpen(...)");
+        }
+
         @Override
         public final ChannelPromise voidPromise() {
             assertEventLoop();
@@ -971,7 +998,7 @@ protected final boolean ensureOpen(ChannelPromise promise) {
                 return true;
             }
 
-            safeSetFailure(promise, ENSURE_OPEN_CLOSED_CHANNEL_EXCEPTION);
+            safeSetFailure(promise, newEnsureOpenException(initialCloseCause));
             return false;
         }
 
diff --git a/transport/src/main/java/io/netty/channel/ExtendedClosedChannelException.java b/transport/src/main/java/io/netty/channel/ExtendedClosedChannelException.java
new file mode 100644
index 00000000000..3b908cd1930
--- /dev/null
+++ b/transport/src/main/java/io/netty/channel/ExtendedClosedChannelException.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2019 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.channel;
+
+import java.nio.channels.ClosedChannelException;
+
+final class ExtendedClosedChannelException extends ClosedChannelException {
+
+    ExtendedClosedChannelException(Throwable cause) {
+        if (cause != null) {
+            initCause(cause);
+        }
+    }
+
+    @Override
+    public Throwable fillInStackTrace() {
+        return this;
+    }
+}


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/AbstractChannelTest.java b/transport/src/test/java/io/netty/channel/AbstractChannelTest.java
index fc5fb9b066d..9d5110ea8d8 100644
--- a/transport/src/test/java/io/netty/channel/AbstractChannelTest.java
+++ b/transport/src/test/java/io/netty/channel/AbstractChannelTest.java
@@ -15,8 +15,12 @@
  */
 package io.netty.channel;
 
+import java.io.IOException;
+import java.net.InetSocketAddress;
 import java.net.SocketAddress;
+import java.nio.channels.ClosedChannelException;
 
+import io.netty.util.NetUtil;
 import org.junit.Test;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
@@ -82,6 +86,69 @@ public void ensureDefaultChannelId() {
         assertTrue(channelId instanceof DefaultChannelId);
     }
 
+    @Test
+    public void testClosedChannelExceptionCarryIOException() throws Exception {
+        final IOException ioException = new IOException();
+        final Channel channel = new TestChannel() {
+            private boolean open = true;
+            private boolean active;
+
+            @Override
+            protected AbstractUnsafe newUnsafe() {
+                return new AbstractUnsafe() {
+                    @Override
+                    public void connect(
+                            SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise) {
+                        active = true;
+                        promise.setSuccess();
+                    }
+                };
+            }
+
+            @Override
+            protected void doClose()  {
+                active = false;
+                open = false;
+            }
+
+            @Override
+            protected void doWrite(ChannelOutboundBuffer in) throws Exception {
+                throw ioException;
+            }
+
+            @Override
+            public boolean isOpen() {
+                return open;
+            }
+
+            @Override
+            public boolean isActive() {
+                return active;
+            }
+        };
+
+        EventLoop loop = new DefaultEventLoop();
+        try {
+            registerChannel(loop, channel);
+            channel.connect(new InetSocketAddress(NetUtil.LOCALHOST, 8888)).sync();
+            assertSame(ioException, channel.writeAndFlush("").await().cause());
+
+            assertClosedChannelException(channel.writeAndFlush(""), ioException);
+            assertClosedChannelException(channel.write(""), ioException);
+            assertClosedChannelException(channel.bind(new InetSocketAddress(NetUtil.LOCALHOST, 8888)), ioException);
+        } finally {
+            channel.close();
+            loop.shutdownGracefully();
+        }
+    }
+
+    private static void assertClosedChannelException(ChannelFuture future, IOException expected)
+            throws InterruptedException {
+        Throwable cause = future.await().cause();
+        assertTrue(cause instanceof ClosedChannelException);
+        assertSame(expected, cause.getCause());
+    }
+
     private static void registerChannel(EventLoop eventLoop, Channel channel) throws Exception {
         DefaultChannelPromise future = new DefaultChannelPromise(channel);
         channel.unsafe().register(eventLoop, future);
@@ -90,11 +157,8 @@ private static void registerChannel(EventLoop eventLoop, Channel channel) throws
 
     private static class TestChannel extends AbstractChannel {
         private static final ChannelMetadata TEST_METADATA = new ChannelMetadata(false);
-        private class TestUnsafe extends AbstractUnsafe {
 
-            @Override
-            public void connect(SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise) { }
-        }
+        private final ChannelConfig config = new DefaultChannelConfig(this);
 
         TestChannel() {
             super(null);
@@ -102,7 +166,7 @@ public void connect(SocketAddress remoteAddress, SocketAddress localAddress, Cha
 
         @Override
         public ChannelConfig config() {
-            return new DefaultChannelConfig(this);
+            return config;
         }
 
         @Override
@@ -122,7 +186,12 @@ public ChannelMetadata metadata() {
 
         @Override
         protected AbstractUnsafe newUnsafe() {
-            return new TestUnsafe();
+            return new AbstractUnsafe() {
+                @Override
+                public void connect(SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise) {
+                    promise.setFailure(new UnsupportedOperationException());
+                }
+            };
         }
 
         @Override
@@ -141,16 +210,16 @@ protected SocketAddress remoteAddress0() {
         }
 
         @Override
-        protected void doBind(SocketAddress localAddress) throws Exception { }
+        protected void doBind(SocketAddress localAddress) { }
 
         @Override
-        protected void doDisconnect() throws Exception { }
+        protected void doDisconnect() { }
 
         @Override
-        protected void doClose() throws Exception { }
+        protected void doClose() { }
 
         @Override
-        protected void doBeginRead() throws Exception { }
+        protected void doBeginRead() { }
 
         @Override
         protected void doWrite(ChannelOutboundBuffer in) throws Exception { }