From 900db0acff95cc8c6334180024ad6d3ab12f220f Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 24 Mar 2016 19:37:43 +0100 Subject: [PATCH] Not attempt to read from fd when channel is closed during read loop. Related to [#5031] Motivation: We need to break out of the read loop for two reasons: - If the input was shutdown in between (which may be the case when the user did it in the fireChannelRead(...) method we should not try to read again to not produce any miss-leading exceptions. - If the user closes the channel we need to ensure we not try to read from it again as the filedescriptor may be re-used already by the OS if the system is handling a lot of concurrent connections and so needs a lot of filedescriptors. If not do this we risk reading data from a filedescriptor that belongs to another socket then the socket that was "wrapped" by this Channel implementation. Modification: Break the reading loop if the input was shutdown from within the channelRead(...) method. Result: No more meaningless exceptions and no risk to read data from wrong socket after the original was closed. Conflicts: transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java --- .../channel/epoll/AbstractEpollStreamChannel.java | 15 +++++++++++++++ .../channel/epoll/EpollSocketChannelTest.java | 7 +++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java index 7d736d67d0..947986097b 100644 --- a/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java +++ b/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java @@ -899,6 +899,21 @@ public abstract class AbstractEpollStreamChannel extends AbstractEpollChannel im // pending data break; } + + if (fd().isInputShutdown()) { + // We need to do this for two reasons: + // + // - If the input was shutdown in between (which may be the case when the user did it in the + // fireChannelRead(...) method we should not try to read again to not produce any + // miss-leading exceptions. + // + // - If the user closes the channel we need to ensure we not try to read from it again as + // the filedescriptor may be re-used already by the OS if the system is handling a lot of + // concurrent connections and so needs a lot of filedescriptors. If not do this we risk + // reading data from a filedescriptor that belongs to another socket then the socket that + // was "wrapped" by this Channel implementation. + break; + } } while (++ messages < maxMessagesPerRead || isRdHup()); pipeline.fireChannelReadComplete(); diff --git a/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketChannelTest.java b/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketChannelTest.java index 109f388f07..a4d35b09b6 100644 --- a/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketChannelTest.java +++ b/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketChannelTest.java @@ -190,10 +190,9 @@ public class EpollSocketChannelTest { private static class ExceptionHandler extends ChannelInboundHandlerAdapter { final AtomicLong count = new AtomicLong(); /** - * We expect to get 2 calls to {@link #exceptionCaught(ChannelHandlerContext, Throwable)}. - * 1 call from BuggyChannelHandler and 1 from closing the channel in this class. + * We expect to get 1 call to {@link #exceptionCaught(ChannelHandlerContext, Throwable)}. */ - final CountDownLatch latch1 = new CountDownLatch(2); + final CountDownLatch latch1 = new CountDownLatch(1); final CountDownLatch latch2 = new CountDownLatch(1); @Override @@ -203,7 +202,7 @@ public class EpollSocketChannelTest { } else { latch2.countDown(); } - // This is expected to throw an exception! + // This should not throw any exception. ctx.close(); } }