From c9512480f4bd644688d3b06fa3ee05b8b3d7f24f Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 26 May 2015 22:23:43 +0200 Subject: [PATCH] [#3785] Correctly handle connection refused with native transport Motivation: Due a bug we not correctly handled connection refused errors and so failed the connect promise with the wrong exception. Beside this we some times even triggered fireChannelActive() which is not correct. Modifications: - Add testcase - correctly detect connect errors Result: Correct and consistent handling. --- .../transport/AbstractTestsuiteTest.java | 2 +- .../socket/SocketConnectionAttemptTest.java | 41 +++++++++++++++++++ .../netty/channel/epoll/EpollEventLoop.java | 18 +++++--- .../java/io/netty/channel/epoll/Native.java | 9 +++- .../EpollSocketConnectionAttemptTest.java | 29 +++++++++++++ 5 files changed, 91 insertions(+), 8 deletions(-) create mode 100644 transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketConnectionAttemptTest.java diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/AbstractTestsuiteTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/AbstractTestsuiteTest.java index 5ec58109df..a43d7bf019 100644 --- a/testsuite/src/main/java/io/netty/testsuite/transport/AbstractTestsuiteTest.java +++ b/testsuite/src/main/java/io/netty/testsuite/transport/AbstractTestsuiteTest.java @@ -57,7 +57,7 @@ public abstract class AbstractTestsuiteTest> { "Running: %s %d of %d with %s", testName.getMethodName(), ++ i, combos.size(), StringUtil.simpleClassName(allocator))); try { - Method m = getClass().getDeclaredMethod( + Method m = getClass().getMethod( TestUtils.testMethodName(testName), clazz); m.invoke(this, cb); } catch (InvocationTargetException ex) { diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java index b8a1d69e87..3c716c8ada 100644 --- a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java +++ b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketConnectionAttemptTest.java @@ -16,10 +16,16 @@ package io.netty.testsuite.transport.socket; import io.netty.bootstrap.Bootstrap; +import io.netty.channel.Channel; import io.netty.channel.ChannelFuture; +import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; import io.netty.channel.ChannelOption; +import io.netty.testsuite.util.TestUtils; +import io.netty.util.NetUtil; +import io.netty.util.concurrent.GlobalEventExecutor; +import io.netty.util.concurrent.Promise; import io.netty.util.internal.SystemPropertyUtil; import io.netty.util.internal.logging.InternalLogger; import io.netty.util.internal.logging.InternalLoggerFactory; @@ -60,6 +66,41 @@ public class SocketConnectionAttemptTest extends AbstractClientSocketTest { } } + @Test(timeout = 30000) + public void testConnectRefused() throws Throwable { + run(); + } + + public void testConnectRefused(Bootstrap cb) throws Throwable { + testConnectRefused0(cb, false); + } + + @Test(timeout = 30000) + public void testConnectRefusedHalfClosure() throws Throwable { + run(); + } + + public void testConnectRefusedHalfClosure(Bootstrap cb) throws Throwable { + testConnectRefused0(cb, true); + } + + private static void testConnectRefused0(Bootstrap cb, boolean halfClosure) throws Throwable { + final Promise errorPromise = GlobalEventExecutor.INSTANCE.newPromise(); + ChannelHandler handler = new ChannelInboundHandlerAdapter() { + @Override + public void channelActive(ChannelHandlerContext ctx) throws Exception { + Channel channel = ctx.channel(); + errorPromise.setFailure(new AssertionError("should have never been called")); + } + }; + + cb.handler(handler); + cb.option(ChannelOption.ALLOW_HALF_CLOSURE, halfClosure); + ChannelFuture future = cb.connect(NetUtil.LOCALHOST, TestUtils.getFreePort()).awaitUninterruptibly(); + assertTrue(future.cause() instanceof ConnectException); + assertNull(errorPromise.cause()); + } + @Test public void testConnectCancellation() throws Throwable { // Check if the test can be executed or should be skipped because of no network/internet connection diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java index 133ed067f4..22b414f3ed 100644 --- a/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java +++ b/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java @@ -309,9 +309,21 @@ final class EpollEventLoop extends SingleThreadEventLoop { AbstractEpollChannel ch = channels.get(fd); if (ch != null) { + // Don't change the ordering of processing EPOLLOUT | EPOLLRDHUP / EPOLLIN if you're not 100% + // sure about it! + // Re-ordering can easily introduce bugs and bad side-effects, as we found out painfully in the + // past. AbstractEpollUnsafe unsafe = (AbstractEpollUnsafe) ch.unsafe(); - // First check if EPOLLRDHUP was set, this will notify us for connection-reset in which case + // First check for EPOLLOUT as we may need to fail the connect ChannelPromise before try + // to read from the file descriptor. + // See https://github.com/netty/netty/issues/3785 + if ((ev & Native.EPOLLOUT) != 0 && ch.isOpen()) { + // Force flush of data as the epoll is writable again + unsafe.epollOutReady(); + } + + // Check if EPOLLRDHUP was set, this will notify us for connection-reset in which case // we may close the channel directly or try to read more data depending on the state of the // Channel and als depending on the AbstractEpollChannel subtype. if ((ev & Native.EPOLLRDHUP) != 0) { @@ -321,10 +333,6 @@ final class EpollEventLoop extends SingleThreadEventLoop { // The Channel is still open and there is something to read. Do it now. unsafe.epollInReady(); } - if ((ev & Native.EPOLLOUT) != 0 && ch.isOpen()) { - // Force flush of data as the epoll is writable again - unsafe.epollOutReady(); - } } else { // We received an event for an fd which we not use anymore. Remove it from the epoll_event set. try { diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/Native.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/Native.java index 91a9f1a792..f1922221f1 100644 --- a/transport-native-epoll/src/main/java/io/netty/channel/epoll/Native.java +++ b/transport-native-epoll/src/main/java/io/netty/channel/epoll/Native.java @@ -25,6 +25,7 @@ import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.SystemPropertyUtil; import java.io.IOException; +import java.net.ConnectException; import java.net.Inet6Address; import java.net.InetAddress; import java.net.InetSocketAddress; @@ -471,7 +472,7 @@ public final class Native { // connect not complete yet need to wait for EPOLLOUT event return false; } - throw newIOException("connect", res); + throw newConnectException("connect", res); } return true; } @@ -486,13 +487,17 @@ public final class Native { // connect still in progress return false; } - throw newIOException("finishConnect", res); + throw newConnectException("finishConnect", res); } return true; } private static native int finishConnect0(int fd); + private static ConnectException newConnectException(String method, int err) { + return new ConnectException(method + "() failed: " + ERRORS[-err]); + } + public static InetSocketAddress remoteAddress(int fd) { byte[] addr = remoteAddress0(fd); // addr may be null if getpeername failed. diff --git a/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketConnectionAttemptTest.java b/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketConnectionAttemptTest.java new file mode 100644 index 0000000000..a4fddfeb38 --- /dev/null +++ b/transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollSocketConnectionAttemptTest.java @@ -0,0 +1,29 @@ +/* + * Copyright 2015 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.epoll; + +import io.netty.bootstrap.Bootstrap; +import io.netty.testsuite.transport.TestsuitePermutation; +import io.netty.testsuite.transport.socket.SocketConnectionAttemptTest; + +import java.util.List; + +public class EpollSocketConnectionAttemptTest extends SocketConnectionAttemptTest { + @Override + protected List> newFactories() { + return EpollSocketTestPermutation.INSTANCE.clientSocket(); + } +}