Repository: netty/netty
Issue: FixedChannelPool doesn't decrease acquiredChannelCount when timeout occurs

Netty version: 4.0.10.Beta5

Context:
When we use `FixedChannelPool` with `AcquireTimeoutAction.NEW`, if timeout occurs, `acquiredChannelCount` increases forever.

Steps to reproduce:
1. Create `FixedChannelPool` with `AcquireTimeoutAction.NEW`
2. Shutdown client network to simulate timeout
3. `acquiredChannelCount` increases forever

Opinion:
- When timeout occurs call `task.acquired()` before delegate to super

``` java
 case NEW:
                timeoutTask = new TimeoutTask() {
                    @Override
                    public void onTimeout(AcquireTask task) {
                        // Increment the acquire count and delegate to super to actually acquire a Channel which will
                        // create a new connetion.
                        ++acquiredChannelCount;
                        task.acquired();

                        FixedChannelPool.super.acquire(task.promise);
                    }
                };
                break;
```
- If possible, encapsulate `++acquiredChnnelCount` in `AcquireListener.acquired` (I'll send a PR for this, #4033)


Issue comments:

Pull request review comments:
I think the old name was a better fit.

I got it, thanks.

change this to `assert acquiredChannelCount >= 0`


Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/pool/FixedChannelPool.java b/transport/src/main/java/io/netty/channel/pool/FixedChannelPool.java
index f23c5404eb0..bd89bd64612 100644
--- a/transport/src/main/java/io/netty/channel/pool/FixedChannelPool.java
+++ b/transport/src/main/java/io/netty/channel/pool/FixedChannelPool.java
@@ -117,7 +117,7 @@ public FixedChannelPool(Bootstrap bootstrap,
      *                              be failed.
      */
     public FixedChannelPool(Bootstrap bootstrap,
-                            ChannelPoolHandler  handler,
+                            ChannelPoolHandler handler,
                             ChannelHealthChecker healthCheck, AcquireTimeoutAction action,
                             final long acquireTimeoutMillis,
                             int maxConnections, int maxPendingAcquires) {
@@ -153,7 +153,7 @@ public void onTimeout(AcquireTask task) {
                     public void onTimeout(AcquireTask task) {
                         // Increment the acquire count and delegate to super to actually acquire a Channel which will
                         // create a new connetion.
-                        ++acquiredChannelCount;
+                        task.acquired();
 
                         FixedChannelPool.super.acquire(task.promise);
                     }
@@ -191,14 +191,14 @@ private void acquire0(final Promise<Channel> promise) {
         assert executor.inEventLoop();
 
         if (acquiredChannelCount < maxConnections) {
-            ++acquiredChannelCount;
-
-            assert acquiredChannelCount > 0;
+            assert acquiredChannelCount >= 0;
 
             // We need to create a new promise as we need to ensure the AcquireListener runs in the correct
             // EventLoop
             Promise<Channel> p = executor.newPromise();
-            p.addListener(new AcquireListener(promise));
+            AcquireListener l = new AcquireListener(promise);
+            l.acquired();
+            p.addListener(l);
             super.acquire(p);
         } else {
             if (pendingAcquireCount >= maxPendingAcquires) {
@@ -271,10 +271,8 @@ private void runTaskQueue() {
                 timeoutFuture.cancel(false);
             }
 
-            task.acquired();
-
             --pendingAcquireCount;
-            ++acquiredChannelCount;
+            task.acquired();
 
             super.acquire(task.promise);
         }
@@ -291,7 +289,7 @@ private final class AcquireTask extends AcquireListener {
         ScheduledFuture<?> timeoutFuture;
 
         public AcquireTask(Promise<Channel> promise) {
-            super(promise, false);
+            super(promise);
             // We need to create a new promise as we need to ensure the AcquireListener runs in the correct
             // EventLoop.
             this.promise = executor.<Channel>newPromise().addListener(this);
@@ -327,12 +325,7 @@ private class AcquireListener implements FutureListener<Channel> {
         protected boolean acquired;
 
         AcquireListener(Promise<Channel> originalPromise) {
-            this(originalPromise, true);
-        }
-
-        protected AcquireListener(Promise<Channel> originalPromise, boolean acquired) {
             this.originalPromise = originalPromise;
-            this.acquired = acquired;
         }
 
         @Override
@@ -353,6 +346,10 @@ public void operationComplete(Future<Channel> future) throws Exception {
         }
 
         public void acquired() {
+            if (acquired) {
+                return;
+            }
+            acquiredChannelCount++;
             acquired = true;
         }
     }


Test patch: