From f176384a729c1d9352c9ed878b9b967ca2f31bf8 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 15 Feb 2019 13:13:17 -0800 Subject: [PATCH] Include the original Exception that caused the Channel to be closed in the ClosedChannelException (#8863) Motivation: To make it easier to understand why a Channel was closed previously and so why the operation failed with a ClosedChannelException we should include the original Exception. Modifications: - Store the original exception that lead to the closed Channel and include it in the ClosedChannelException that is used to fail the operation. - Add unit test Result: Fixes https://github.com/netty/netty/issues/8862. --- .../io/netty/channel/AbstractChannel.java | 45 ++++++++-- .../ExtendedClosedChannelException.java | 32 +++++++ .../io/netty/channel/AbstractChannelTest.java | 89 ++++++++++++++++--- 3 files changed, 147 insertions(+), 19 deletions(-) create mode 100644 transport/src/main/java/io/netty/channel/ExtendedClosedChannelException.java diff --git a/transport/src/main/java/io/netty/channel/AbstractChannel.java b/transport/src/main/java/io/netty/channel/AbstractChannel.java index 11c14a9624..8f8fe2f787 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 abstract class AbstractChannel extends DefaultAttributeMap implements Cha // 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 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha 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 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha * 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 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha } } + 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 @@ public abstract class AbstractChannel extends DefaultAttributeMap implements Cha 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 0000000000..3b908cd193 --- /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; + } +} diff --git a/transport/src/test/java/io/netty/channel/AbstractChannelTest.java b/transport/src/test/java/io/netty/channel/AbstractChannelTest.java index fc5fb9b066..9d5110ea8d 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 class AbstractChannelTest { 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 @@ public class AbstractChannelTest { 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 class AbstractChannelTest { @Override public ChannelConfig config() { - return new DefaultChannelConfig(this); + return config; } @Override @@ -122,7 +186,12 @@ public class AbstractChannelTest { @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 @@ public class AbstractChannelTest { } @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 { }