Repository: netty/netty
Issue: AbstractBootstrap#initAndRegister crash doesn't notify promise
Labels: defect

`initAndRegister` can possibly crash, eg:

```
java.net.SocketException: Too many open files
    at sun.nio.ch.Net.socket0(Native Method)
    at sun.nio.ch.Net.socket(Net.java:411)
    at sun.nio.ch.Net.socket(Net.java:404)
    at sun.nio.ch.SocketChannelImpl.<init>(SocketChannelImpl.java:105)
    at sun.nio.ch.SelectorProviderImpl.openSocketChannel(SelectorProviderImpl.java:60)
    at io.netty.channel.socket.nio.NioSocketChannel.newSocket(NioSocketChannel.java:60)
    ... 64 common frames omitted
Wrapped by: io.netty.channel.ChannelException: Failed to open a socket.
    at io.netty.channel.socket.nio.NioSocketChannel.newSocket(NioSocketChannel.java:62)
    at io.netty.channel.socket.nio.NioSocketChannel.<init>(NioSocketChannel.java:72)
    at org.asynchttpclient.netty.channel.NioSocketChannelFactory.newChannel(NioSocketChannelFactory.java:25)
    at org.asynchttpclient.netty.channel.NioSocketChannelFactory.newChannel(NioSocketChannelFactory.java:19)
    at io.netty.bootstrap.AbstractBootstrap.initAndRegister(AbstractBootstrap.java:308)
```

The problem is that the `newSocket` isn't monitored with a try/catch, so promise isn't notify and the caller crash.

IMHO, such Exception should be dealt with in the promise. Users shouldn't have to combine 2 different Exception handling mechanisms and both wrap the call with a try/catch AND check the future.


Issue comments:
@slandelle you are right... let me fix.

Awesome, thanks!
I can provide a PR if you want, but I first wanted to be sure it was a bug and not intended behavior.

@slandelle a pr would be awesome! Please also add a test that we catch when `ChannelFactory.newChannel(...)` throws and notify the promise.

PR on its way

Actually, having a look at `BootstrapTest#testLateRegistrationConnect` makes me think that `connect` is expected to possibly throw exceptions.

I can't say I like this behavior, but it is what it is, and IMHO can't be changed/fixed outside a new version.

WDYT?

Should Exception be thrown AND listener notified?

@slandelle I think you misread the test case. It only throws because we do `syncUninterruptibly()`.

My bad...

@slandelle no worries!

Done, see #5388

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


Pull request review comments:
Please create the exception before the bootstrap and just throw it. This way you can use `assertSame(...)` below to check if the same exception. This is more robust compared to check for the message.

remove empty line

See below.

See above.


Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java
index 519446c9e67..8513f20cb4c 100644
--- a/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java
+++ b/transport/src/main/java/io/netty/bootstrap/AbstractBootstrap.java
@@ -314,11 +314,15 @@ public void operationComplete(ChannelFuture future) throws Exception {
     }
 
     final ChannelFuture initAndRegister() {
-        final Channel channel = channelFactory.newChannel();
+        Channel channel = null;
         try {
+            channel = channelFactory.newChannel();
             init(channel);
         } catch (Throwable t) {
-            channel.unsafe().closeForcibly();
+            if (channel != null) {
+                // channel can be null if newChannel crashed (eg SocketException("too many open files"))
+                channel.unsafe().closeForcibly();
+            }
             // as the Channel is not registered yet we need to force the usage of the GlobalEventExecutor
             return new DefaultChannelPromise(channel, GlobalEventExecutor.INSTANCE).setFailure(t);
         }


Test patch:
diff --git a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java
index 81292f7cc5d..db00b984144 100644
--- a/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java
+++ b/transport/src/test/java/io/netty/bootstrap/BootstrapTest.java
@@ -69,7 +69,6 @@ public static void destroy() {
 
     @Test(timeout = 10000)
     public void testBindDeadLock() throws Exception {
-
         final Bootstrap bootstrapA = new Bootstrap();
         bootstrapA.group(groupA);
         bootstrapA.channel(LocalChannel.class);
@@ -106,7 +105,6 @@ public void run() {
 
     @Test(timeout = 10000)
     public void testConnectDeadLock() throws Exception {
-
         final Bootstrap bootstrapA = new Bootstrap();
         bootstrapA.group(groupA);
         bootstrapA.channel(LocalChannel.class);
@@ -234,7 +232,6 @@ public void testLateRegistrationConnect() throws Exception {
 
     @Test
     public void testAsyncResolutionSuccess() throws Exception {
-
         final Bootstrap bootstrapA = new Bootstrap();
         bootstrapA.group(groupA);
         bootstrapA.channel(LocalChannel.class);
@@ -253,7 +250,6 @@ public void testAsyncResolutionSuccess() throws Exception {
 
     @Test
     public void testAsyncResolutionFailure() throws Exception {
-
         final Bootstrap bootstrapA = new Bootstrap();
         bootstrapA.group(groupA);
         bootstrapA.channel(LocalChannel.class);
@@ -275,6 +271,28 @@ public void testAsyncResolutionFailure() throws Exception {
         assertThat(connectFuture.channel().isOpen(), is(false));
     }
 
+    @Test
+    public void testChannelFactoryFailureNotifiesPromise() throws Exception {
+        final RuntimeException exception = new RuntimeException("newChannel crash");
+
+        final Bootstrap bootstrap = new Bootstrap()
+                .handler(dummyHandler)
+                .group(groupA)
+                .channelFactory(new ChannelFactory<Channel>() {
+            @Override
+            public Channel newChannel() {
+                throw exception;
+            }
+        });
+
+        ChannelFuture connectFuture = bootstrap.connect(LocalAddress.ANY);
+
+        // Should fail with the RuntimeException.
+        assertThat(connectFuture.await(10000), is(true));
+        assertThat(connectFuture.cause(), sameInstance((Throwable) exception));
+        assertThat(connectFuture.channel(), is(nullValue()));
+    }
+
     private static final class DelayedEventLoopGroup extends DefaultEventLoop {
         @Override
         public ChannelFuture register(final Channel channel, final ChannelPromise promise) {