Repository: netty/netty
Issue: SimpleChannelPool#notifyConnect() may leak Channels
Labels: defect

Hey,

I just stumbled onto this one. The `SimpleChannelPool#notifyConnect()` method will leak Channels if the user cancelled the Promise. 

``` java
private static void notifyConnect(ChannelFuture future, Promise<Channel> promise) {
        if (future.isSuccess()) {
            // This will leak the Channel if the Promise is complete
            promise.setSuccess(future.channel());
        } else {
            promise.setFailure(future.cause());
        }
    }
```

``` java
private static void notifyConnect(ChannelFuture future, Promise<Channel> promise) {
        if (future.isSuccess()) {
            // Possible fix
            Channel channel = future.channel();
            if (!promise.trySuccess(channel)) {
                release(channel);
            }
        } else {
            promise.setFailure(future.cause());
        }
    }
```

``` java
2016-07-19 14:14:48:400 EDT [32647,NIO-ConnectorThread-0,] WARN DefaultPromise - An exception was thrown by io.netty.channel.pool.SimpleChannelPool$2.operationComplete()
java.lang.IllegalStateException: complete already: DefaultPromise@78ba125f(failure: java.util.concurrent.CancellationException)
    at io.netty.util.concurrent.DefaultPromise.setSuccess(DefaultPromise.java:105)
    at io.netty.channel.pool.SimpleChannelPool.notifyConnect(SimpleChannelPool.java:159)
    at io.netty.channel.pool.SimpleChannelPool.access$000(SimpleChannelPool.java:42)
    at io.netty.channel.pool.SimpleChannelPool$2.operationComplete(SimpleChannelPool.java:134)
    at io.netty.channel.pool.SimpleChannelPool$2.operationComplete(SimpleChannelPool.java:131)
    at io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:514)
    at io.netty.util.concurrent.DefaultPromise.notifyListeners0(DefaultPromise.java:507)
    at io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:486)
    at io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:427)
    at io.netty.util.concurrent.DefaultPromise.trySuccess(DefaultPromise.java:111)
    at io.netty.channel.DefaultChannelPromise.trySuccess(DefaultChannelPromise.java:82)
    at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.fulfillConnectPromise(AbstractNioChannel.java:300)
    at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.finishConnect(AbstractNioChannel.java:335)
    at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:588)
    at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:512)
    at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:426)
    at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:398)
    at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:877)
    at java.lang.Thread.run(Thread.java:745)
```


Issue comments:
@rkapsi good catch! Let me come up with a fix

@rkapsi PTAL https://github.com/netty/netty/pull/5557

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


Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/pool/SimpleChannelPool.java b/transport/src/main/java/io/netty/channel/pool/SimpleChannelPool.java
index 85767f32c6c..9fcf0d8d6f0 100644
--- a/transport/src/main/java/io/netty/channel/pool/SimpleChannelPool.java
+++ b/transport/src/main/java/io/netty/channel/pool/SimpleChannelPool.java
@@ -154,9 +154,13 @@ public void run() {
         return promise;
     }
 
-    private static void notifyConnect(ChannelFuture future, Promise<Channel> promise) {
+    private void notifyConnect(ChannelFuture future, Promise<Channel> promise) {
         if (future.isSuccess()) {
-            promise.setSuccess(future.channel());
+            Channel channel = future.channel();
+            if (!promise.trySuccess(channel)) {
+                // Promise was completed in the meantime (like cancelled), just release the channel again
+                release(channel);
+            }
         } else {
             promise.setFailure(future.cause());
         }


Test patch: