Skip to content

Commit 24c63e1

Browse files
garyrussellartembilan
authored andcommitted
GH-1034: DMLC: Detect target channel changed
Fixes #1034 If the connection factory refreshed the target connection, the DMLC is not made aware of it and so we never consume from the new channel. **cherry-pick to all 2.x**
1 parent b4bd47b commit 24c63e1

File tree

2 files changed

+73
-3
lines changed

2 files changed

+73
-3
lines changed

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

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import org.springframework.amqp.core.Message;
4848
import org.springframework.amqp.core.MessageProperties;
4949
import org.springframework.amqp.core.Queue;
50+
import org.springframework.amqp.rabbit.connection.ChannelProxy;
5051
import org.springframework.amqp.rabbit.connection.Connection;
5152
import org.springframework.amqp.rabbit.connection.ConnectionFactory;
5253
import org.springframework.amqp.rabbit.connection.ConnectionFactoryUtils;
@@ -473,7 +474,8 @@ private void checkConsumers(long now) {
473474
synchronized (this.consumersMonitor) {
474475
consumersToCancel = this.consumers.stream()
475476
.filter(consumer -> {
476-
boolean open = consumer.getChannel().isOpen() && !consumer.isAckFailed();
477+
boolean open = consumer.getChannel().isOpen() && !consumer.isAckFailed()
478+
&& !consumer.targetChanged();
477479
if (open && this.messagesPerAck > 1) {
478480
try {
479481
consumer.ackIfNecessary(now);
@@ -871,6 +873,8 @@ final class SimpleConsumer extends DefaultConsumer {
871873

872874
private final long ackTimeout = DirectMessageListenerContainer.this.ackTimeout;
873875

876+
private final Channel targetChannel;
877+
874878
private int pendingAcks;
875879

876880
private long lastAck = System.currentTimeMillis();
@@ -889,11 +893,17 @@ final class SimpleConsumer extends DefaultConsumer {
889893

890894
private volatile boolean ackFailed;
891895

892-
private SimpleConsumer(Connection connection, Channel channel, String queue) {
896+
SimpleConsumer(Connection connection, Channel channel, String queue) {
893897
super(channel);
894898
this.connection = connection;
895899
this.queue = queue;
896900
this.ackRequired = !getAcknowledgeMode().isAutoAck() && !getAcknowledgeMode().isManual();
901+
if (channel instanceof ChannelProxy) {
902+
this.targetChannel = ((ChannelProxy) channel).getTargetChannel();
903+
}
904+
else {
905+
this.targetChannel = null;
906+
}
897907
}
898908

899909
private String getQueue() {
@@ -930,6 +940,15 @@ boolean isAckFailed() {
930940
return this.ackFailed;
931941
}
932942

943+
/**
944+
* True if the channel is a proxy and the underlying channel has changed.
945+
* @return true if the condition exists.
946+
*/
947+
boolean targetChanged() {
948+
return this.targetChannel != null
949+
&& !((ChannelProxy) getChannel()).getTargetChannel().equals(this.targetChannel);
950+
}
951+
933952
/**
934953
* Increment and return the current epoch for this consumer; consumersMonitor must
935954
* be held.

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

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.junit.Test;
4242
import org.mockito.Mockito;
4343

44+
import org.springframework.amqp.core.AcknowledgeMode;
4445
import org.springframework.amqp.core.MessageListener;
4546
import org.springframework.amqp.rabbit.connection.ChannelProxy;
4647
import org.springframework.amqp.rabbit.connection.Connection;
@@ -175,7 +176,7 @@ else if (i.getArgument(0).equals(17L)) {
175176
}
176177
Thread.sleep(200);
177178
consumer.get().handleDelivery("consumerTag", envelope(16), props, body);
178-
// should get 2 acks #10 and #6 (timeout)
179+
// should get 2 acks #10 and #16 (timeout)
179180
assertThat(latch2.await(10, TimeUnit.SECONDS)).isTrue();
180181
consumer.get().handleDelivery("consumerTag", envelope(17), props, body);
181182
verify(channel).basicAck(10L, true);
@@ -309,6 +310,56 @@ public void testMonitorCancelsAfterBadAckEvenIfChannelReportsOpen() throws Excep
309310
container.stop();
310311
}
311312

313+
@Test
314+
public void testMonitorCancelsAfterTargetChannelChanges() throws Exception {
315+
ConnectionFactory connectionFactory = mock(ConnectionFactory.class);
316+
Connection connection = mock(Connection.class);
317+
ChannelProxy channel = mock(ChannelProxy.class);
318+
Channel rabbitChannel1 = mock(Channel.class);
319+
Channel rabbitChannel2 = mock(Channel.class);
320+
AtomicReference<Channel> target = new AtomicReference<>(rabbitChannel1);
321+
willAnswer(inv -> {
322+
return target.get();
323+
}).given(channel).getTargetChannel();
324+
325+
given(connectionFactory.createConnection()).willReturn(connection);
326+
given(connection.createChannel(anyBoolean())).willReturn(channel);
327+
given(channel.isOpen()).willReturn(true);
328+
given(channel.queueDeclarePassive(Mockito.anyString()))
329+
.willAnswer(invocation -> mock(AMQP.Queue.DeclareOk.class));
330+
AtomicReference<Consumer> consumer = new AtomicReference<>();
331+
final CountDownLatch latch1 = new CountDownLatch(1);
332+
final CountDownLatch latch2 = new CountDownLatch(1);
333+
willAnswer(inv -> {
334+
consumer.set(inv.getArgument(6));
335+
latch1.countDown();
336+
return "consumerTag";
337+
}).given(channel).basicConsume(anyString(), anyBoolean(), anyString(), anyBoolean(), anyBoolean(),
338+
anyMap(), any(Consumer.class));
339+
340+
willAnswer(inv -> {
341+
consumer.get().handleCancelOk("consumerTag");
342+
latch2.countDown();
343+
return null;
344+
}).given(channel).basicCancel("consumerTag");
345+
346+
DirectMessageListenerContainer container = new DirectMessageListenerContainer(connectionFactory);
347+
container.setQueueNames("test");
348+
container.setPrefetchCount(2);
349+
container.setMonitorInterval(100);
350+
container.setMessageListener(msg -> {
351+
target.set(rabbitChannel2);
352+
});
353+
container.setAcknowledgeMode(AcknowledgeMode.MANUAL);
354+
container.afterPropertiesSet();
355+
container.start();
356+
357+
assertThat(latch1.await(10, TimeUnit.SECONDS)).isTrue();
358+
consumer.get().handleDelivery("consumerTag", envelope(1L), new BasicProperties(), new byte[1]);
359+
assertThat(latch2.await(10, TimeUnit.SECONDS)).isTrue();
360+
container.stop();
361+
}
362+
312363
private Envelope envelope(long tag) {
313364
return new Envelope(tag, false, "", "");
314365
}

0 commit comments

Comments
 (0)