Skip to content

Commit 70ba65f

Browse files
committed
GH-2593: Reliably shutdown SMLC
Fixes: #2593 * Add `activeObjectCounter` release into the `BlockingQueueConsumer.handleCancelOk()` in reply to the `basicCancel()` call * Adjust `BlockingQueueConsumer.basicCancel()` to call `RabbitUtils.closeMessageConsumer()` to setisfy transactional context * Adjust `SimpleMessageListenerContainerIntegrationTests` to eventually setisfy to the transaction rollback when container is shuted down * Add new tests into the `ContainerShutDownTests` to verify the listener containers are not blocked waiting on the `cancelationLock` **Cherry-pick to `3.0.x`**
1 parent 6cc91b5 commit 70ba65f

File tree

5 files changed

+60
-19
lines changed

5 files changed

+60
-19
lines changed

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -1177,7 +1177,7 @@ protected boolean isForceStop() {
11771177
/**
11781178
* Set to true to stop the container after the current message(s) are processed and
11791179
* requeue any prefetched. Useful when using exclusive or single-active consumers.
1180-
* @param forceStop true to stop when current messsage(s) are processed.
1180+
* @param forceStop true to stop when current message(s) are processed.
11811181
* @since 2.4.14
11821182
*/
11831183
public void setForceStop(boolean forceStop) {

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -465,11 +465,10 @@ protected void basicCancel() {
465465

466466
protected void basicCancel(boolean expected) {
467467
this.normalCancel = expected;
468-
getConsumerTags().forEach(consumerTag -> {
469-
if (this.channel.isOpen()) {
470-
RabbitUtils.cancel(this.channel, consumerTag);
471-
}
472-
});
468+
Collection<String> consumerTags = getConsumerTags();
469+
if (!CollectionUtils.isEmpty(consumerTags)) {
470+
RabbitUtils.closeMessageConsumer(this.channel, consumerTags, this.transactional);
471+
}
473472
this.cancelled.set(true);
474473
this.abortStarted = System.currentTimeMillis();
475474
}
@@ -1007,6 +1006,7 @@ public void handleCancelOk(String consumerTag) {
10071006
+ "); " + BlockingQueueConsumer.this);
10081007
}
10091008
this.canceled = true;
1009+
BlockingQueueConsumer.this.activeObjectCounter.release(BlockingQueueConsumer.this);
10101010
}
10111011

10121012
@Override

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

+43-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2017-2019 the original author or authors.
2+
* Copyright 2017-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -30,11 +30,13 @@
3030
import org.springframework.amqp.rabbit.core.RabbitTemplate;
3131
import org.springframework.amqp.rabbit.junit.RabbitAvailable;
3232
import org.springframework.amqp.utils.test.TestUtils;
33+
import org.springframework.util.StopWatch;
3334

3435
import com.rabbitmq.client.AMQP.BasicProperties;
3536

3637
/**
3738
* @author Gary Russell
39+
* @author Artem Bilan
3840
* @since 2.0
3941
*
4042
*/
@@ -56,7 +58,6 @@ public void testUninterruptibleListenerDMLC() throws Exception {
5658
public void testUninterruptibleListener(AbstractMessageListenerContainer container) throws Exception {
5759
CachingConnectionFactory cf = new CachingConnectionFactory("localhost");
5860
container.setConnectionFactory(cf);
59-
container.setShutdownTimeout(500);
6061
container.setQueueNames("test.shutdown");
6162
final CountDownLatch latch = new CountDownLatch(1);
6263
final CountDownLatch testEnded = new CountDownLatch(1);
@@ -91,11 +92,49 @@ public void testUninterruptibleListener(AbstractMessageListenerContainer contain
9192
assertThat(channels).hasSize(2);
9293
}
9394
finally {
95+
testEnded.countDown();
9496
container.stop();
95-
assertThat(channels).hasSize(1);
97+
cf.destroy();
98+
}
99+
}
100+
101+
@Test
102+
public void consumersCorrectlyCancelledOnShutdownSMLC() throws Exception {
103+
SimpleMessageListenerContainer container = new SimpleMessageListenerContainer();
104+
consumersCorrectlyCancelledOnShutdown(container);
105+
}
96106

107+
@Test
108+
public void consumersCorrectlyCancelledOnShutdownDMLC() throws Exception {
109+
DirectMessageListenerContainer container = new DirectMessageListenerContainer();
110+
consumersCorrectlyCancelledOnShutdown(container);
111+
}
112+
113+
private void consumersCorrectlyCancelledOnShutdown(AbstractMessageListenerContainer container)
114+
throws InterruptedException {
115+
116+
CachingConnectionFactory cf = new CachingConnectionFactory("localhost");
117+
container.setConnectionFactory(cf);
118+
container.setQueueNames("test.shutdown");
119+
container.setMessageListener(m -> {
120+
});
121+
final CountDownLatch startLatch = new CountDownLatch(1);
122+
container.setApplicationEventPublisher(e -> {
123+
if (e instanceof AsyncConsumerStartedEvent) {
124+
startLatch.countDown();
125+
}
126+
});
127+
container.start();
128+
try {
129+
assertThat(startLatch.await(30, TimeUnit.SECONDS)).isTrue();
130+
StopWatch stopWatch = new StopWatch();
131+
stopWatch.start();
132+
container.shutdown();
133+
stopWatch.stop();
134+
assertThat(stopWatch.getTotalTimeMillis()).isLessThan(3000);
135+
}
136+
finally {
97137
cf.destroy();
98-
testEnded.countDown();
99138
}
100139
}
101140

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

+3-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2019 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@
1818

1919
import static org.assertj.core.api.Assertions.assertThat;
2020
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
21+
import static org.awaitility.Awaitility.await;
2122

2223
import java.util.ArrayList;
2324
import java.util.Arrays;
@@ -314,7 +315,7 @@ private void doListenerWithExceptionTest(CountDownLatch latch, MessageListener l
314315
container.shutdown();
315316
}
316317
if (acknowledgeMode.isTransactionAllowed()) {
317-
assertThat(template.receiveAndConvert(queue.getName())).isNotNull();
318+
await().untilAsserted(() -> assertThat(template.receiveAndConvert(queue.getName())).isNotNull());
318319
}
319320
else {
320321
assertThat(template.receiveAndConvert(queue.getName())).isNull();

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

+6-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -50,6 +50,7 @@
5050
import java.util.Iterator;
5151
import java.util.List;
5252
import java.util.Map;
53+
import java.util.Objects;
5354
import java.util.Set;
5455
import java.util.concurrent.CountDownLatch;
5556
import java.util.concurrent.Executor;
@@ -517,7 +518,7 @@ public void testWithConnectionPerListenerThread() throws Exception {
517518
waitForConsumersToStop(consumers);
518519
Set<?> allocatedConnections = TestUtils.getPropertyValue(ccf, "allocatedConnections", Set.class);
519520
assertThat(allocatedConnections).hasSize(2);
520-
assertThat(ccf.getCacheProperties().get("openConnections")).isEqualTo("1");
521+
assertThat(ccf.getCacheProperties().get("openConnections")).isEqualTo("2");
521522
}
522523

523524
@Test
@@ -807,15 +808,15 @@ private Answer<Object> messageToConsumer(final Channel mockChannel, final Simple
807808

808809
}
809810

810-
private void waitForConsumersToStop(Set<?> consumers) throws Exception {
811+
private void waitForConsumersToStop(Set<?> consumers) {
811812
with().pollInterval(Duration.ofMillis(10)).atMost(Duration.ofSeconds(10))
812813
.until(() -> consumers.stream()
813814
.map(consumer -> TestUtils.getPropertyValue(consumer, "consumer"))
814-
.allMatch(c -> c == null));
815+
.allMatch(Objects::isNull));
815816
}
816817

817818
@SuppressWarnings("serial")
818-
private class TestTransactionManager extends AbstractPlatformTransactionManager {
819+
private static class TestTransactionManager extends AbstractPlatformTransactionManager {
819820

820821
@Override
821822
protected void doBegin(Object transaction, TransactionDefinition definition) throws TransactionException {

0 commit comments

Comments
 (0)