Repository: netty/netty
Issue: FixedChannelPool#close() can return before idle channels are closed

### Expected behavior
`FixedChannelPool#close()` should return only after all idle channels are closed. Such behavior is expected after https://github.com/netty/netty/pull/7927. Before this PR, method was asynchronous and provided no such guarantees.

### Actual behavior
`FixedChannelPool#close()` offloads actual closing of idle channels to `GlobalEventExecutor` and can return before the submitted task completes.

### Steps to reproduce

1. Create a `FixedChannelPool`
2. Acquire and release a channel, but keep a reference to it
3. Call `#close()` on the pool
4. Assert channel close future is done

### Minimal yet complete reproducer code (or URL to code)
The following test can be added to the `FixedChannelPoolTest` class to reproduce the problem:
```java
@Test
public void testClose() {
    LocalAddress addr = new LocalAddress(LOCAL_ADDR_ID);
    Bootstrap cb = new Bootstrap();
    cb.remoteAddress(addr);
    cb.group(group).channel(LocalChannel.class);

    ServerBootstrap sb = new ServerBootstrap();
    sb.group(group)
      .channel(LocalServerChannel.class)
      .childHandler(new ChannelInitializer<LocalChannel>() {
        @Override
        public void initChannel(LocalChannel ch) throws Exception {
            ch.pipeline().addLast(new ChannelInboundHandlerAdapter());
        }
    });

    // Start server
    Channel sc = sb.bind(addr).syncUninterruptibly().channel();

    FixedChannelPool pool = new FixedChannelPool(cb, new TestChannelPoolHandler(), 2);
    Channel channel = pool.acquire().syncUninterruptibly().getNow();
    pool.release(channel).syncUninterruptibly().getNow();

    pool.close();

    assertTrue(channel.closeFuture().isDone()); // <<-- failing assertion

    sc.close().syncUninterruptibly();
}
```

### Netty version
4.1.26+

### JVM version (e.g. `java -version`)
```
java version "1.8.0_181"
Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)
```

### OS version (e.g. `uname -a`)
```
Darwin MacBook-Pro.local 17.7.0 Darwin Kernel Version 17.7.0: Fri Jul  6 19:54:51 PDT 2018; root:xnu-4570.71.3~2/RELEASE_X86_64 x86_64
```

### More details
We use Netty in a client app which hangs extremely infrequently during shutdown when updated from 4.1.22 to 4.1.26+. Shutdown sequence looks roughly like this:

```java
fixedChannelPool.close();
eventLoopGroup.shutdownGracefully();
eventLoopGroup.terminationFuture().awaitUninterruptibly(); // this line can block forever
```

Unfortunately, I'm unable to reproduce such hanging with a test. Was only able to catch it once in a debugger with the following code: https://gist.github.com/lutovich/dc1fe9414e728d12b29b65c15727df93.

Stacktraces of the two blocked threads look like:

```
"main@1" prio=5 tid=0x1 nid=NA waiting
  java.lang.Thread.State: WAITING
	  at java.lang.Object.wait(Object.java:-1)
	  at java.lang.Object.wait(Object.java:502)
	  at io.netty.util.concurrent.DefaultPromise.awaitUninterruptibly(DefaultPromise.java:253)
	  at io.netty.util.concurrent.DefaultPromise.awaitUninterruptibly(DefaultPromise.java:33)
	  at io.netty.channel.pool. NettyPoolCloseTest.poolClose(NettyIT.java:62)
          ...
```

```
"globalEventExecutor-1-1@2059" prio=5 tid=0x1a nid=NA waiting
  java.lang.Thread.State: WAITING
	  at java.lang.Object.wait(Object.java:-1)
	  at java.lang.Object.wait(Object.java:502)
	  at io.netty.util.concurrent.DefaultPromise.awaitUninterruptibly(DefaultPromise.java:253)
	  at io.netty.channel.DefaultChannelPromise.awaitUninterruptibly(DefaultChannelPromise.java:137)
	  at io.netty.channel.DefaultChannelPromise.awaitUninterruptibly(DefaultChannelPromise.java:30)
	  at io.netty.channel.pool.SimpleChannelPool.close(SimpleChannelPool.java:398)
	  at io.netty.channel.pool.FixedChannelPool.access$1301(FixedChannelPool.java:40)
	  at io.netty.channel.pool.FixedChannelPool$6.run(FixedChannelPool.java:480)
	  at io.netty.util.concurrent.GlobalEventExecutor$TaskRunner.run(GlobalEventExecutor.java:248)
	  at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	  at java.lang.Thread.run(Thread.java:748)
```

First one is the blocked executing `clientGroup.terminationFuture().awaitUninterruptibly()`. Second is blocked executing `channel.close().awaitUninterruptibly()` in `SimpleChannelPool#close()`. I also have a heap dump of a hanging test run and can upload it somewhere, if needed.

I **think** the problem happens because `FixedChannelPool#close()` might exit before all pooled channels are closed and closing of the event loop group will race with closing of pooled channels.

`FixedChannelPool#close()` offloads actual closing of idle channels to the `GlobalEventExecutor`, which executes `SimpleChannelPool#close()`. So `FixedChannelPool#close()` can return to the caller before idle channels are closed. This might be a scenario that causes hanging:

1. Given a `FixedChannelPool` with an idle connection
2. Code calls `FixedChannelPool#close()` which adds a task to close all idle channels to the `GlobalEventExecutor` and returns
3. Code calls `NioEventLoopGroup#close()` which now races with the `GlobalEventExecutor` who tries to close the channel
4. Somehow, `GlobalEventExecutor` manages to add a close task to the event executor while it is shutting down and a message "An event executor terminated with non-empty task queue (1)" is logged. Tbh, I can't understand how this step is possible by looking at Netty code :(
5. `GlobalEventExecutor` is blocked in `SimpleChannelPool#close()` waiting for the channel close future which will never be notified because executor has been terminated
6. Main thread is blocked on `NioEventLoopGroup#terminationFuture()`. Termination future in  `NioEventLoopGroup` relies on `GlobalEventExecutor` to get notifications but `GlobalEventExecutor` is blocked, as described in step 5.
7. `GlobalEventExecutor` is blocked and will never receive the notification about the channel being closed. It also contains a queued task to notify the event loop termination future, which will never get executed.

If `FixedChannelPool#close()` returns only after all idle channels are closed then there will be no race in step 3 and the problem will hopefully be solved.

Issue comments:
Here is a tentative fix for the described problem: https://github.com/lutovich/netty/commit/bb6b371f0e974814ce1175283c0c06e660180202. I can make it into a PR if the change is acceptable.
@lutovich I think this looks good from a quick look. Can you open a pr ?
Thanks, @normanmaurer. I created a PR.
Fixed in https://github.com/netty/netty/pull/9226.

Pull request review comments:
@lutovich wouldn't that throw and exception when called from within the `EventLoop` as it may possible deadlock ?
This appears racy; more than one channel could see isEmpty() == true. So this should be trySuccess()
It looks like the previous code called awaitUninterruptibly.
Even when on the EventLoop ?
Yeah, that's a problem. Will fix
Previous code executed `channel.close().awaitUninterruptibly()` unconditionally. This can be a problem, I think. Should be enough to not call `awaitUninterruptibly()` when in event loop that is responsible for `Future` returned by `closeIdleChannels()`, right? Will fix this
`sync...()` ?
`trySuccess(...)` ?
@normanmaurer, why was it suggested to swap to trySuccess here?
@ejona86 couldn't this race ?
A pet peeve of mine is when code behaves differently when running on the event loop...

In this code, implicitly no longer waiting for completion when on the event loop is broken, as the caller could have been expecting the blocking behavior (as that's what started this series of bug fixes!). When having one Channel interact with another Channel, it is random whether the two Channels share the same event loop. Code should either always block or never block, always schedule a task or never schedule a task, etc. I know that large portions of the Netty codebase don't agree with me on that :) (although that may change in Netty 5).

There's also a secondary fallacy involved here when checking inEventLoop(), since it uses await/syncUninterruptibly. Just because you aren't in _the_ event loop doesn't mean you aren't in _a_ event loop. It seems that issue was added in #7927 and it is possible to deadlock (if you are worried about being called from an event loop. Which is seems like you are concerned about that case since you checked inEventLoop to begin with).

I agree we're between a rock and a hard place, but I wonder if this is going about it the wrong way.

To me either 1) the method should always awaitUninterruptibly, and we just say you shouldn't call it from an event loop or 2) we make the method async again and solve the original bug another way. For (2) we could maybe create a new method that lets you wait until the cleanup is done by returning a future, or awaiting the pool's termination.

I may not understand these classes well enough, but it appears the code in this PR is even worse than the more general problem, because the `inEventLoop()` check is bonkers. It _looks_ like a normal check, but in actuality it doesn't protect anything. The `executor` was retrieved from `nextEventLoop()` which is gotten from `group.next()`. So it is a _random_ loop. Combined with the fact that we don't _directly_ schedule anything on that loop, it is sort of a senseless. Note that we do _indirectly_ schedule something on every event loop in the group: the listener. So if this was ever run from the event loop group it would deadlock (n-1)/n of the time (n is number of loops in group) and if run outside _this_ event loop group it would block still and thus could deadlock, but whether it actually deadlocks would depend on the full sum of behavior in the system.
Doesn't look like it. It is a private method that is only used in one spot. The passed in promise is not used before this method and was not leaked to any other objects.
@ejona86 I agree, changing method behavior depending on the caller thread is far from ideal. IMHO option (2) would be the best way to solve this and related problems. Not sure if it is fine wrt to backwards compatibility to restore the async behavior of `ChannelPool#close()` and add another interface method like `ChannelPool#closeFuture()`.

@normanmaurer what do you think?
This is scary to me.  Modifying the list while you are iterating over it is not a good idea.  Instead, you should keep track of how many operations have completed, and complete the future then.  Consider using CountDownLatch instead.
This is not a good idea either.  If there were failures with closing the channel, this code throws away that potentially useful information.  You need to keep track of the errors, and add them to a suppressed list is there is more than one.
Sync uninterruptibly is not a good idea.  When the program is trying to shutdown (which is presumably when this method would be called), getting an Interruption means give up.  This should either declare it throws IE or wrap it in a runtime exception.
Could you please elaborate on why is this a bad idea? Set of channels is thread-safe and channels are removed from it by event loop threads that execute close future callbacks. It could happen that channel is removed by the current thread but this should not cause any problems.

I'm not sure how CountDownLatch can be used here. It only allows code to block waiting for the count to be zero. Blocking is not desirable here, I believe.

It could be an `AtomicInteger` for counting closed channels. However, I feel like the current code is a bit more explicit about what's going on. Set of channels to be closed is "snapshotted" and every closed channel is removed from the set.
IMHO propagating idle channel closing errors from `SimpleChannelPool.close()` is not very caller-friendly. How would caller handle such error? Would it be useful to propagate an exception with 99 suppressed exceptions if pool had 100 idle channels and all of them failed to close?

Apache commons-pool swallows resource close errors when closed. Javadoc is [here](https://github.com/apache/commons-pool/blob/POOL_2_5_0/src/main/java/org/apache/commons/pool2/ObjectPool.java#L174) and impl is [here](https://github.com/apache/commons-pool/blob/POOL_2_5_0/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java#L655-L667). HikariCP also just loggs such erros [here](https://github.com/brettwooldridge/HikariCP/blob/HikariCP-3.2.0/src/main/java/com/zaxxer/hikari/pool/HikariPool.java#L430-L447).

Callers that care about channel close errors can always subscribe to close future of every created channel and gather the errors. Logging errors in the pool could be nice to have.
This method could exit before all the channels are closed if it is responsive to interrupts. Event loop threads will still be running even though the current thread is free to go. I'm not sure if it's okay or not. Re-interrupt + runtime exception would be the best option wrt to backward compatibility - in my opinion.
> Could you please elaborate on why is this a bad idea?

Sure, Most collections throw a ConcurrrentModification exception if you modify them while reading.   The reason is because the iterator doesn't have a well defined meaning, or would be feasible to implement.  Concurrent collections have to special case such modifications, but it's still not "obviously" correct.   

When I read this code, I had to do a double take to make sure there weren't bugs with the map being shared, or how the concurrent modifications were happening, how remove was implemented in CHM.  It's not quick to understand.

Also, do you really need a map?  Atomic Integer avoids that allocation.
Since the Pool creates the channels, it's responsible for cleaning them up.  It would not be a correct abstraction boundary for callers to watch the close future for a Channel they didn't create.  


SGTM

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 5ca376f88d2..af927dbf7dd 100644
--- a/transport/src/main/java/io/netty/channel/pool/FixedChannelPool.java
+++ b/transport/src/main/java/io/netty/channel/pool/FixedChannelPool.java
@@ -20,7 +20,6 @@
 import io.netty.util.concurrent.EventExecutor;
 import io.netty.util.concurrent.Future;
 import io.netty.util.concurrent.FutureListener;
-import io.netty.util.concurrent.GlobalEventExecutor;
 import io.netty.util.concurrent.Promise;
 import io.netty.util.internal.ObjectUtil;
 import io.netty.util.internal.ThrowableUtil;
@@ -444,18 +443,21 @@ public void acquired() {
     @Override
     public void close() {
         if (executor.inEventLoop()) {
-            close0();
+            failPendingAcquireOperations();
         } else {
             executor.submit(new Runnable() {
                 @Override
                 public void run() {
-                    close0();
+                    failPendingAcquireOperations();
                 }
             }).awaitUninterruptibly();
         }
+        super.closeIdleChannels(executor);
     }
 
-    private void close0() {
+    private void failPendingAcquireOperations() {
+        assert executor.inEventLoop();
+
         if (!closed) {
             closed = true;
             for (;;) {
@@ -471,15 +473,6 @@ private void close0() {
             }
             acquiredChannelCount.set(0);
             pendingAcquireCount = 0;
-
-            // Ensure we dispatch this on another Thread as close0 will be called from the EventExecutor and we need
-            // to ensure we will not block in a EventExecutor.
-            GlobalEventExecutor.INSTANCE.execute(new Runnable() {
-                @Override
-                public void run() {
-                    FixedChannelPool.super.close();
-                }
-            });
         }
     }
 }
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 6fcfd4443fa..f0741cf5199 100644
--- a/transport/src/main/java/io/netty/channel/pool/SimpleChannelPool.java
+++ b/transport/src/main/java/io/netty/channel/pool/SimpleChannelPool.java
@@ -22,13 +22,17 @@
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.EventLoop;
 import io.netty.util.AttributeKey;
+import io.netty.util.concurrent.EventExecutor;
 import io.netty.util.concurrent.Future;
 import io.netty.util.concurrent.FutureListener;
 import io.netty.util.concurrent.Promise;
 import io.netty.util.internal.PlatformDependent;
 import io.netty.util.internal.ThrowableUtil;
 
+import java.util.Collections;
 import java.util.Deque;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import static io.netty.util.internal.ObjectUtil.*;
 
@@ -155,7 +159,7 @@ protected boolean releaseHealthCheck() {
 
     @Override
     public final Future<Channel> acquire() {
-        return acquire(bootstrap.config().group().next().<Channel>newPromise());
+        return acquire(nextEventLoop().<Channel>newPromise());
     }
 
     @Override
@@ -389,13 +393,47 @@ protected boolean offerChannel(Channel channel) {
 
     @Override
     public void close() {
+        closeIdleChannels(nextEventLoop());
+    }
+
+    void closeIdleChannels(EventExecutor executor) {
+        Promise<Void> result = executor.newPromise();
+        closeIdleChannels(result);
+        if (!executor.inEventLoop()) {
+            result.syncUninterruptibly();
+        }
+    }
+
+    private void closeIdleChannels(final Promise<Void> result) {
+        final Set<Channel> channelsToClose = Collections.newSetFromMap(new ConcurrentHashMap<Channel, Boolean>());
         for (;;) {
             Channel channel = pollChannel();
             if (channel == null) {
                 break;
             }
-            // Just ignore any errors that are reported back from close().
-            channel.close().awaitUninterruptibly();
+            channelsToClose.add(channel);
+        }
+        if (channelsToClose.isEmpty()) {
+            // no idle channels in the pool - nothing to close
+            result.trySuccess(null);
+        }
+
+        for (final Channel channel : channelsToClose) {
+            channel.close().addListener(new ChannelFutureListener() {
+                @Override
+                public void operationComplete(ChannelFuture future) {
+                    channelsToClose.remove(channel);
+                    if (channelsToClose.isEmpty()) {
+                        // last channel was closed - can complete the result promise
+                        // it might already be completed by a concurrently executing listener so use trySuccess
+                        result.trySuccess(null);
+                    }
+                }
+            });
         }
     }
+
+    private EventLoop nextEventLoop() {
+        return bootstrap.config().group().next();
+    }
 }


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/pool/FixedChannelPoolTest.java b/transport/src/test/java/io/netty/channel/pool/FixedChannelPoolTest.java
index bbf1debeac5..7b284f431cb 100644
--- a/transport/src/test/java/io/netty/channel/pool/FixedChannelPoolTest.java
+++ b/transport/src/test/java/io/netty/channel/pool/FixedChannelPoolTest.java
@@ -34,6 +34,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 
 public class FixedChannelPoolTest {
@@ -346,6 +347,110 @@ public void initChannel(LocalChannel ch) throws Exception {
         sc.close().syncUninterruptibly();
     }
 
+    @Test
+    public void testCloseWithIdleChannels() throws Exception {
+        testCloseWithIdleChannels(false);
+    }
+
+    @Test
+    public void testCloseWithIdleChannelsInEventLoop() throws Exception {
+        testCloseWithIdleChannels(true);
+    }
+
+    @Test
+    public void testCloseWithOutstandingAcquireRequests() throws Exception {
+        testCloseWithOutstandingAcquireRequests(false);
+    }
+
+    @Test
+    public void testCloseWithOutstandingAcquireRequestsInEventLoop() throws Exception {
+        testCloseWithOutstandingAcquireRequests(true);
+    }
+
+    private void testCloseWithOutstandingAcquireRequests(boolean closeInEventLoop) throws Exception {
+        LocalAddress addr = new LocalAddress(LOCAL_ADDR_ID);
+        Bootstrap cb = new Bootstrap();
+        cb.remoteAddress(addr);
+        cb.group(group).channel(LocalChannel.class);
+
+        ServerBootstrap sb = new ServerBootstrap();
+        sb.group(group)
+          .channel(LocalServerChannel.class)
+          .childHandler(new ChannelInitializer<LocalChannel>() {
+              @Override
+              public void initChannel(LocalChannel ch) {
+                  ch.pipeline().addLast(new ChannelInboundHandlerAdapter());
+              }
+          });
+
+        Channel sc = sb.bind(addr).syncUninterruptibly().channel();
+
+        FixedChannelPool pool = new FixedChannelPool(cb, new TestChannelPoolHandler(), 1);
+        pool.acquire().get(); // acquire the only available channel
+
+        Future<Channel> acquireRequest1 = pool.acquire();
+        Future<Channel> acquireRequest2 = pool.acquire();
+
+        assertFalse(acquireRequest1.isDone());
+        assertFalse(acquireRequest2.isDone());
+
+        closePool(pool, closeInEventLoop);
+
+        assertTrue(acquireRequest1.isDone());
+        assertThat(acquireRequest1.cause(), instanceOf(IllegalStateException.class));
+        assertTrue(acquireRequest2.isDone());
+        assertThat(acquireRequest2.cause(), instanceOf(IllegalStateException.class));
+
+        sc.close().syncUninterruptibly();
+    }
+
+    private void testCloseWithIdleChannels(boolean closeInEventLoop) throws Exception {
+        LocalAddress addr = new LocalAddress(LOCAL_ADDR_ID);
+        Bootstrap cb = new Bootstrap();
+        cb.remoteAddress(addr);
+        cb.group(group).channel(LocalChannel.class);
+
+        ServerBootstrap sb = new ServerBootstrap();
+        sb.group(group)
+          .channel(LocalServerChannel.class)
+          .childHandler(new ChannelInitializer<LocalChannel>() {
+              @Override
+              public void initChannel(LocalChannel ch) {
+                  ch.pipeline().addLast(new ChannelInboundHandlerAdapter());
+              }
+          });
+
+        Channel sc = sb.bind(addr).syncUninterruptibly().channel();
+
+        FixedChannelPool pool = new FixedChannelPool(cb, new TestChannelPoolHandler(), 2);
+        Channel channel1 = pool.acquire().get();
+        Channel channel2 = pool.acquire().get();
+        pool.release(channel1).get();
+        pool.release(channel2).get();
+
+        closePool(pool, closeInEventLoop);
+
+        assertTrue(channel1.closeFuture().isSuccess());
+        assertTrue(channel2.closeFuture().isSuccess());
+
+        sc.close().syncUninterruptibly();
+    }
+
+    private void closePool(final FixedChannelPool pool, final boolean inEventLoop) throws Exception {
+        if (inEventLoop) {
+            EventLoopGroup eventLoopGroup = pool.bootstrap().config().group();
+            Future<?> poolCloseFuture = eventLoopGroup.submit(new Runnable() {
+                @Override
+                public void run() {
+                    pool.close();
+                }
+            });
+            poolCloseFuture.get();
+        } else {
+            pool.close();
+        }
+    }
+
     private static final class TestChannelPoolHandler extends AbstractChannelPoolHandler {
         @Override
         public void channelCreated(Channel ch) throws Exception {
diff --git a/transport/src/test/java/io/netty/channel/pool/SimpleChannelPoolTest.java b/transport/src/test/java/io/netty/channel/pool/SimpleChannelPoolTest.java
index a91790c38be..8fe562dc810 100644
--- a/transport/src/test/java/io/netty/channel/pool/SimpleChannelPoolTest.java
+++ b/transport/src/test/java/io/netty/channel/pool/SimpleChannelPoolTest.java
@@ -20,6 +20,7 @@
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.ChannelInitializer;
+import io.netty.channel.DefaultEventLoopGroup;
 import io.netty.channel.EventLoopGroup;
 import io.netty.channel.local.LocalAddress;
 import io.netty.channel.local.LocalChannel;
@@ -27,9 +28,7 @@
 import io.netty.channel.local.LocalServerChannel;
 import io.netty.util.concurrent.Future;
 import org.hamcrest.CoreMatchers;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 
 import java.util.Queue;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -237,7 +236,9 @@ public void initChannel(LocalChannel ch) throws Exception {
 
     @Test
     public void testBootstrap() {
-        final SimpleChannelPool pool = new SimpleChannelPool(new Bootstrap(), new CountingChannelPoolHandler());
+        final EventLoopGroup group = new DefaultEventLoopGroup();
+        final Bootstrap bootstrap = new Bootstrap().group(group);
+        final SimpleChannelPool pool = new SimpleChannelPool(bootstrap, new CountingChannelPoolHandler());
 
         try {
             // Checking for the actual bootstrap object doesn't make sense here, since the pool uses a copy with a
@@ -245,26 +246,32 @@ public void testBootstrap() {
             assertNotNull(pool.bootstrap());
         } finally {
             pool.close();
+            group.shutdownGracefully();
         }
     }
 
     @Test
     public void testHandler() {
         final ChannelPoolHandler handler = new CountingChannelPoolHandler();
-        final SimpleChannelPool pool = new SimpleChannelPool(new Bootstrap(), handler);
+        final EventLoopGroup group = new DefaultEventLoopGroup();
+        final Bootstrap bootstrap = new Bootstrap().group(group);
+        final SimpleChannelPool pool = new SimpleChannelPool(bootstrap, handler);
 
         try {
             assertSame(handler, pool.handler());
         } finally {
             pool.close();
+            group.shutdownGracefully();
         }
     }
 
     @Test
     public void testHealthChecker() {
         final ChannelHealthChecker healthChecker = ChannelHealthChecker.ACTIVE;
+        final EventLoopGroup group = new DefaultEventLoopGroup();
+        final Bootstrap bootstrap = new Bootstrap().group(group);
         final SimpleChannelPool pool = new SimpleChannelPool(
-                new Bootstrap(),
+                bootstrap,
                 new CountingChannelPoolHandler(),
                 healthChecker);
 
@@ -272,13 +279,16 @@ public void testHealthChecker() {
             assertSame(healthChecker, pool.healthChecker());
         } finally {
             pool.close();
+            group.shutdownGracefully();
         }
     }
 
     @Test
     public void testReleaseHealthCheck() {
+        final EventLoopGroup group = new DefaultEventLoopGroup();
+        final Bootstrap bootstrap = new Bootstrap().group(group);
         final SimpleChannelPool healthCheckOnReleasePool = new SimpleChannelPool(
-                new Bootstrap(),
+                bootstrap,
                 new CountingChannelPoolHandler(),
                 ChannelHealthChecker.ACTIVE,
                 true);
@@ -290,7 +300,7 @@ public void testReleaseHealthCheck() {
         }
 
         final SimpleChannelPool noHealthCheckOnReleasePool = new SimpleChannelPool(
-                new Bootstrap(),
+                bootstrap,
                 new CountingChannelPoolHandler(),
                 ChannelHealthChecker.ACTIVE,
                 false);
@@ -300,5 +310,59 @@ public void testReleaseHealthCheck() {
         } finally {
             noHealthCheckOnReleasePool.close();
         }
+
+        group.shutdownGracefully();
+    }
+
+    @Test
+    public void testCloseWithIdleChannels() throws Exception {
+        testCloseWithIdleChannelsInEventLoop(false);
+    }
+
+    @Test
+    public void testCloseWithIdleChannelsInEventLoop() throws Exception {
+        testCloseWithIdleChannelsInEventLoop(true);
+    }
+
+    private void testCloseWithIdleChannelsInEventLoop(boolean closeInEventLoop) throws Exception {
+        EventLoopGroup group = new DefaultEventLoopGroup();
+        LocalAddress address = new LocalAddress(LOCAL_ADDR_ID);
+        Bootstrap cb = new Bootstrap().remoteAddress(address).group(group).channel(LocalChannel.class);
+
+        ServerBootstrap sb = new ServerBootstrap();
+        sb.group(group)
+          .channel(LocalServerChannel.class)
+          .childHandler(new ChannelInitializer<LocalChannel>() {
+              @Override
+              public void initChannel(LocalChannel ch) {
+                  ch.pipeline().addLast(new ChannelInboundHandlerAdapter());
+              }
+          });
+
+        Channel sc = sb.bind(address).sync().channel();
+
+        final ChannelPool pool = new SimpleChannelPool(cb, new CountingChannelPoolHandler());
+
+        Channel channel1 = pool.acquire().get();
+        Channel channel2 = pool.acquire().get();
+        pool.release(channel1).get();
+        pool.release(channel2).get();
+
+        if (closeInEventLoop) {
+            group.submit(new Runnable() {
+                @Override
+                public void run() {
+                    pool.close();
+                }
+            }).get();
+        } else {
+            pool.close();
+        }
+
+        assertTrue(channel1.closeFuture().isSuccess());
+        assertTrue(channel2.closeFuture().isSuccess());
+
+        sc.close().get();
+        group.shutdownGracefully();
     }
 }