GH-1034: DMLC: Cancel consumer after failed ack

Resolves #1034

The monitor task now cancels the consumer after a failed ack/nack,
whether or not the channel `isOpen()` returns true.

Test with a mock channel that stays open after a failed ack.

**cherry-pick to 2.1.x, 2.0.x**

* Fix tests removing AssertJ dependency

Author: garyrussell

spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainer.java

@@ -387,7 +387,7 @@ protected void actualStart() throws Exception {
 			synchronized (this.consumersMonitor) {
 				consumersToCancel = this.consumers.stream()
 						.filter(c -> {
-							boolean open = c.getChannel().isOpen();
+							boolean open = c.getChannel().isOpen() && !c.isAckFailed();
 							if (open && this.messagesPerAck > 1) {
 								try {
 									c.ackIfNecessary(now);
@@ -502,7 +502,6 @@ protected void actualStart() throws Exception {
 						}
 					}
 				}
-
 			});
 		}
 		else {
@@ -717,7 +716,7 @@ private void cancelConsumer(SimpleConsumer consumer) {
 				this.logger.debug("Canceling " + consumer);
 			}
 			synchronized (consumer) {
-				consumer.canceled = true;
+				consumer.setCanceled(true);
 				if (this.messagesPerAck > 1) {
 					consumer.ackIfNecessary(0L);
 				}
@@ -791,6 +790,8 @@ final class SimpleConsumer extends DefaultConsumer {
 
 		private volatile boolean canceled;
 
+		private volatile boolean ackFailed;
+
 		private SimpleConsumer(Connection connection, Channel channel, String queue) {
 			super(channel);
 			this.connection = connection;
@@ -815,6 +816,23 @@ int getEpoch() {
 			return this.epoch;
 		}
 
+		/**
+		 * Set to true to indicate this consumer is canceled and should send any pending
+		 * acks.
+		 * @param canceled the canceled to set
+		 */
+		void setCanceled(boolean canceled) {
+			this.canceled = canceled;
+		}
+
+		/**
+		 * True if an ack/nack failed (probably due to a closed channel).
+		 * @return the ackFailed
+		 */
+		boolean isAckFailed() {
+			return this.ackFailed;
+		}
+
 		/**
 		 * Increment and return the current epoch for this consumer; consumersMonitor must
 		 * be held.
@@ -966,6 +984,7 @@ else if (!isChannelTransacted() || isLocallyTransacted) {
 				}
 			}
 			catch (Exception e) {
+				this.ackFailed = true;
 				this.logger.error("Error acking", e);
 			}
 		}
@@ -976,7 +995,7 @@ else if (!isChannelTransacted() || isLocallyTransacted) {
 		 * @param now the current time.
 		 * @throws IOException if one occurs.
 		 */
-		private synchronized void ackIfNecessary(long now) throws IOException {
+		synchronized void ackIfNecessary(long now) throws IOException {
 			if (this.pendingAcks >= this.messagesPerAck || (
 					this.pendingAcks > 0 && (now - this.lastAck > this.ackTimeout || this.canceled))) {
 				sendAck(now);

spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainerMockTests.java

@@ -28,6 +28,7 @@
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.BDDMockito.willAnswer;
+import static org.mockito.BDDMockito.willThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -42,6 +43,7 @@
 import org.junit.Test;
 import org.mockito.Mockito;
 
+import org.springframework.amqp.core.MessageListener;
 import org.springframework.amqp.rabbit.connection.ChannelProxy;
 import org.springframework.amqp.rabbit.connection.Connection;
 import org.springframework.amqp.rabbit.connection.ConnectionFactory;
@@ -265,6 +267,50 @@ public void testRemoveQueuesWhileNotConnected() throws Exception {
 		container.stop();
 	}
 
+	@Test
+	public void testMonitorCancelsAfterBadAckEvenIfChannelReportsOpen() throws Exception {
+		ConnectionFactory connectionFactory = mock(ConnectionFactory.class);
+		Connection connection = mock(Connection.class);
+		ChannelProxy channel = mock(ChannelProxy.class);
+		Channel rabbitChannel = mock(Channel.class);
+		given(channel.getTargetChannel()).willReturn(rabbitChannel);
+
+		given(connectionFactory.createConnection()).willReturn(connection);
+		given(connection.createChannel(anyBoolean())).willReturn(channel);
+		given(channel.isOpen()).willReturn(true);
+		given(channel.queueDeclarePassive(Mockito.anyString()))
+				.willAnswer(invocation -> mock(AMQP.Queue.DeclareOk.class));
+		AtomicReference<Consumer> consumer = new AtomicReference<>();
+		final CountDownLatch latch1 = new CountDownLatch(1);
+		final CountDownLatch latch2 = new CountDownLatch(1);
+		willAnswer(inv -> {
+			consumer.set(inv.getArgument(6));
+			latch1.countDown();
+			return "consumerTag";
+		}).given(channel).basicConsume(anyString(), anyBoolean(), anyString(), anyBoolean(), anyBoolean(),
+						anyMap(), any(Consumer.class));
+
+		willThrow(new RuntimeException("bad ack")).given(channel).basicAck(1L, false);
+		willAnswer(inv -> {
+			consumer.get().handleCancelOk("consumerTag");
+			latch2.countDown();
+			return null;
+		}).given(channel).basicCancel("consumerTag");
+
+		DirectMessageListenerContainer container = new DirectMessageListenerContainer(connectionFactory);
+		container.setQueueNames("test");
+		container.setPrefetchCount(2);
+		container.setMonitorInterval(100);
+		container.setMessageListener(mock(MessageListener.class));
+		container.afterPropertiesSet();
+		container.start();
+
+		assertTrue(latch1.await(10, TimeUnit.SECONDS));
+		consumer.get().handleDelivery("consumerTag", envelope(1L), new BasicProperties(), new byte[1]);
+		assertTrue(latch2.await(10, TimeUnit.SECONDS));
+		container.stop();
+	}
+
 	private Envelope envelope(long tag) {
 		return new Envelope(tag,  false, "", "");
 	}