Fix RejectedExecutionException during close

Resolves #994

`CCF.destroy()` abruptly shuts down the `channelsExecutor` (if present).
This can cause `RejectedExecutionException` because the publisher
callback channel uses the same executor to schedule nacks if necessary.

- Use an `ActiveObjectCounter` to keep track of in-flight async closes
- Wait until there are not active objects before calling `shutDown()` (instead of `shutDownNow()`)
- Wait until all queued tasks are complete
- Add `PublisherCallbackChannelFactory` to make testing possible

**cherry-pick to 2.1.x**

# Conflicts:
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/PublisherCallbackChannelImpl.java

Author: garyrussell

spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/CachingConnectionFactory.java

@@ -50,6 +50,7 @@
 import org.springframework.amqp.AmqpApplicationContextClosedException;
 import org.springframework.amqp.AmqpException;
 import org.springframework.amqp.AmqpTimeoutException;
+import org.springframework.amqp.rabbit.listener.ActiveObjectCounter;
 import org.springframework.amqp.support.ConditionalExceptionLogger;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.jmx.export.annotation.ManagedAttribute;
@@ -105,6 +106,8 @@ public class CachingConnectionFactory extends AbstractConnectionFactory
 
 	private static final String DEFAULT_DEFERRED_POOL_PREFIX = "spring-rabbit-deferred-pool-";
 
+	private static final int CHANNEL_EXEC_SHUTDOWN_TIMEOUT = 30;
+
 	/**
 	 * Create a unique ID for the pool.
 	 */
@@ -159,6 +162,8 @@ public enum CacheMode {
 	/** Synchronization monitor for the shared Connection. */
 	private final Object connectionMonitor = new Object();
 
+	private final ActiveObjectCounter<Channel> inFlightAsyncCloses = new ActiveObjectCounter<>();
+
 	private long channelCheckoutTimeout = 0;
 
 	private CacheMode cacheMode = CacheMode.CHANNEL;
@@ -177,6 +182,8 @@ public enum CacheMode {
 
 	private ConditionalExceptionLogger closeExceptionLogger = new DefaultChannelCloseLogger();
 
+	private PublisherCallbackChannelFactory publisherChannelFactory = PublisherCallbackChannelImpl.factory();
+
 	private volatile boolean active = true;
 
 	private volatile boolean initialized;
@@ -430,6 +437,16 @@ public void setCloseExceptionLogger(ConditionalExceptionLogger closeExceptionLog
 		}
 	}
 
+	/**
+	 * Set the factory to use to create {@link PublisherCallbackChannel} instances.
+	 * @param publisherChannelFactory the factory.
+	 * @since 2.1.6
+	 */
+	public void setPublisherChannelFactory(PublisherCallbackChannelFactory publisherChannelFactory) {
+		Assert.notNull(publisherChannelFactory, "'publisherChannelFactory' cannot be null");
+		this.publisherChannelFactory = publisherChannelFactory;
+	}
+
 	@Override
 	public void afterPropertiesSet() {
 		this.initialized = true;
@@ -651,8 +668,8 @@ else if (this.cacheMode == CacheMode.CONNECTION) {
 		return null; // NOSONAR doCreate will throw an exception
 	}
 
-	private Channel doCreateBareChannel(ChannelCachingConnectionProxy connection, boolean transactional) {
-		Channel channel = connection.createBareChannel(transactional);
+	private Channel doCreateBareChannel(ChannelCachingConnectionProxy conn, boolean transactional) {
+		Channel channel = conn.createBareChannel(transactional);
 		if (this.publisherConfirms || this.simplePublisherConfirms) {
 			try {
 				channel.confirmSelect();
@@ -663,7 +680,7 @@ private Channel doCreateBareChannel(ChannelCachingConnectionProxy connection, bo
 		}
 		if ((this.publisherConfirms || this.publisherReturns)
 				&& !(channel instanceof PublisherCallbackChannelImpl)) {
-			channel = new PublisherCallbackChannelImpl(channel, getChannelsExecutor());
+			channel = this.publisherChannelFactory.createChannel(channel, getChannelsExecutor());
 		}
 		if (channel != null) {
 			channel.addShutdownListener(this);
@@ -819,7 +836,18 @@ public final void destroy() {
 		if (getContextStopped()) {
 			this.stopped = true;
 			if (this.channelsExecutor != null) {
-				this.channelsExecutor.shutdownNow();
+				try {
+					if (!this.inFlightAsyncCloses.await(CHANNEL_EXEC_SHUTDOWN_TIMEOUT, TimeUnit.SECONDS)) {
+						this.logger.warn("Async closes are still in-flight: " + this.inFlightAsyncCloses.getCount());
+					}
+					this.channelsExecutor.shutdown();
+					if (!this.channelsExecutor.awaitTermination(CHANNEL_EXEC_SHUTDOWN_TIMEOUT, TimeUnit.SECONDS)) {
+						this.logger.warn("Channel executor failed to shut down");
+					}
+				}
+				catch (@SuppressWarnings("unused") InterruptedException e) {
+					Thread.currentThread().interrupt();
+				}
 			}
 		}
 	}
@@ -1276,37 +1304,46 @@ private void physicalClose() throws IOException, TimeoutException {
 		private void asyncClose() {
 			ExecutorService executorService = getChannelsExecutor();
 			final Channel channel = CachedChannelInvocationHandler.this.target;
-			executorService.execute(() -> {
-				try {
-					if (CachingConnectionFactory.this.publisherConfirms) {
-						channel.waitForConfirmsOrDie(ASYNC_CLOSE_TIMEOUT);
-					}
-					else {
-						Thread.sleep(ASYNC_CLOSE_TIMEOUT);
-					}
-				}
-				catch (InterruptedException e1) {
-					Thread.currentThread().interrupt();
-				}
-				catch (Exception e2) {
-				}
-				finally {
+			CachingConnectionFactory.this.inFlightAsyncCloses.add(channel);
+			try {
+				executorService.execute(() -> {
 					try {
-						channel.close();
-					}
-					catch (IOException e3) {
+						if (CachingConnectionFactory.this.publisherConfirms) {
+							channel.waitForConfirmsOrDie(ASYNC_CLOSE_TIMEOUT);
+						}
+						else {
+							Thread.sleep(ASYNC_CLOSE_TIMEOUT);
+						}
 					}
-					catch (AlreadyClosedException e4) {
+					catch (InterruptedException e1) {
+						Thread.currentThread().interrupt();
 					}
-					catch (TimeoutException e5) {
+					catch (Exception e2) {
 					}
-					catch (ShutdownSignalException e6) {
-						if (!RabbitUtils.isNormalShutdown(e6)) {
-							logger.debug("Unexpected exception on deferred close", e6);
+					finally {
+						try {
+							channel.close();
+						}
+						catch (IOException e3) {
+						}
+						catch (AlreadyClosedException e4) {
+						}
+						catch (TimeoutException e5) {
+						}
+						catch (ShutdownSignalException e6) {
+							if (!RabbitUtils.isNormalShutdown(e6)) {
+								logger.debug("Unexpected exception on deferred close", e6);
+							}
+						}
+						finally {
+							CachingConnectionFactory.this.inFlightAsyncCloses.release(channel);
 						}
 					}
-				}
-			});
+				});
+			}
+			catch (@SuppressWarnings("unused") RuntimeException e) {
+				CachingConnectionFactory.this.inFlightAsyncCloses.release(channel);
+			}
 		}
 
 	}

spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/PublisherCallbackChannelFactory.java

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2019 the original author or authors.
+ *
+ * Licensed 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
+ *
+ *      https://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 org.springframework.amqp.rabbit.connection;
+
+import java.util.concurrent.ExecutorService;
+
+import com.rabbitmq.client.Channel;
+
+/**
+ * A factory for {@link PublisherCallbackChannel}s.
+ *
+ * @author Gary Russell
+ * @since 2.1.6
+ *
+ */
+@FunctionalInterface
+public interface PublisherCallbackChannelFactory {
+
+	/**
+	 * Create a {@link PublisherCallbackChannel} instance based on the provided delegate
+	 * and executor.
+	 * @param delegate the delegate channel.
+	 * @param executor the executor.
+	 * @return the channel.
+	 */
+	PublisherCallbackChannel createChannel(Channel delegate, ExecutorService executor);
+
+}

spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/PublisherCallbackChannelImpl.java

@@ -1125,4 +1125,8 @@ public String toString() {
 		return "PublisherCallbackChannelImpl: " + this.delegate.toString();
 	}
 
+	public static PublisherCallbackChannelFactory factory() {
+		return (channel, exec) -> new PublisherCallbackChannelImpl(channel, exec);
+	}
+
 }

spring-rabbit/src/test/java/org/springframework/amqp/rabbit/connection/CachingConnectionFactoryTests.java

@@ -28,6 +28,7 @@
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.BDDMockito.given;
 import static org.mockito.BDDMockito.willAnswer;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doAnswer;
@@ -36,6 +37,7 @@
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -54,6 +56,7 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
@@ -72,6 +75,8 @@
 import org.springframework.amqp.AmqpTimeoutException;
 import org.springframework.amqp.rabbit.connection.CachingConnectionFactory.CacheMode;
 import org.springframework.amqp.utils.test.TestUtils;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.event.ContextClosedEvent;
 import org.springframework.test.util.ReflectionTestUtils;
 
 import com.rabbitmq.client.Address;
@@ -1655,4 +1660,61 @@ public void testReturnsNormalCloseDeferredClose() throws Exception {
 		Thread.sleep(6000);
 	}
 
+	@Test
+	public void testOrderlyShutDown() throws Exception {
+		com.rabbitmq.client.ConnectionFactory mockConnectionFactory = mock(com.rabbitmq.client.ConnectionFactory.class);
+		com.rabbitmq.client.Connection mockConnection = mock(com.rabbitmq.client.Connection.class);
+		Channel mockChannel = mock(Channel.class);
+
+		given(mockConnectionFactory.newConnection((ExecutorService) isNull(), anyString())).willReturn(mockConnection);
+		given(mockConnection.createChannel()).willReturn(mockChannel);
+		given(mockChannel.isOpen()).willReturn(true);
+		given(mockConnection.isOpen()).willReturn(true);
+
+		CachingConnectionFactory ccf = new CachingConnectionFactory(mockConnectionFactory);
+		ccf.setPublisherConfirms(true);
+		ApplicationContext ac = mock(ApplicationContext.class);
+		ccf.setApplicationContext(ac);
+		PublisherCallbackChannel pcc = mock(PublisherCallbackChannel.class);
+		given(pcc.isOpen()).willReturn(true);
+		AtomicReference<ExecutorService> executor = new AtomicReference<>();
+		AtomicBoolean rejected = new AtomicBoolean(true);
+		CountDownLatch closeLatch = new CountDownLatch(1);
+		ccf.setPublisherChannelFactory((channel, exec) -> {
+			executor.set(spy(exec));
+			return pcc;
+		});
+		willAnswer(invoc -> {
+			try {
+				executor.get().execute(() -> {
+				});
+				rejected.set(false);
+			}
+			catch (@SuppressWarnings("unused") RejectedExecutionException e) {
+				rejected.set(true);
+			}
+			closeLatch.countDown();
+			return null;
+		}).given(pcc).close();
+		Channel channel = ccf.createConnection().createChannel(false);
+		ExecutorService closeExec = Executors.newSingleThreadExecutor();
+		CountDownLatch asyncClosingLatch = new CountDownLatch(1);
+		closeExec.execute(() -> {
+			RabbitUtils.setPhysicalCloseRequired(channel, true);
+			try {
+				channel.close();
+				asyncClosingLatch.countDown();
+			}
+			catch (@SuppressWarnings("unused") IOException | TimeoutException e) {
+				// ignore
+			}
+		});
+		assertThat(asyncClosingLatch.await(10, TimeUnit.SECONDS)).isTrue();
+		ccf.onApplicationEvent(new ContextClosedEvent(ac));
+		ccf.destroy();
+		assertThat(closeLatch.await(10, TimeUnit.SECONDS)).isTrue();
+		assertThat(rejected.get()).isFalse();
+		closeExec.shutdownNow();
+	}
+
 }