Skip to content

Commit 5ff484a

Browse files
committed
Fix cancel/receive race in messageSourceToFlux for PollableChannel
Replace synchronized cancellation flag with pending-message reclaim on a shared scheduler and Sinks.One tryEmitValue to detect cancelled emission. Handle failed PollableChannel re-queue via nack when send returns false. Fixes #11262 Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
1 parent bb5a638 commit 5ff484a

2 files changed

Lines changed: 83 additions & 25 deletions

File tree

spring-integration-core/src/main/java/org/springframework/integration/util/IntegrationReactiveUtils.java

Lines changed: 40 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.springframework.integration.util;
1818

1919
import java.time.Duration;
20+
import java.util.concurrent.atomic.AtomicReference;
2021
import java.util.concurrent.locks.LockSupport;
2122

2223
import io.micrometer.context.ContextSnapshotFactory;
@@ -27,6 +28,7 @@
2728
import reactor.core.publisher.Flux;
2829
import reactor.core.publisher.Mono;
2930
import reactor.core.publisher.Sinks;
31+
import reactor.core.scheduler.Scheduler;
3032
import reactor.core.scheduler.Schedulers;
3133
import reactor.util.context.Context;
3234
import reactor.util.context.ContextView;
@@ -129,29 +131,18 @@ public static ContextView captureReactorContext() {
129131
*/
130132
@SuppressWarnings("NullAway")
131133
public static <T> Flux<Message<T>> messageSourceToFlux(MessageSource<T> messageSource) {
132-
return Mono.
133-
<Message<T>>create(monoSink -> {
134-
Object cancellationMonitor = new Object();
135-
boolean[] cancelled = {false};
136-
monoSink.onCancel(() -> {
137-
synchronized (cancellationMonitor) {
138-
cancelled[0] = true;
139-
}
140-
});
141-
monoSink.onRequest(value -> {
142-
synchronized (cancellationMonitor) {
143-
if (cancelled[0]) {
144-
return;
145-
}
146-
Message<T> message = messageSource.receive();
147-
if (cancelled[0]) {
148-
handleUndeliveredMessage(messageSource, message);
149-
return;
150-
}
151-
monoSink.success(message);
152-
}
153-
});
154-
})
134+
Scheduler scheduler = Schedulers.single();
135+
return Mono.defer(() -> {
136+
AtomicReference<Message<?>> pendingMessage = new AtomicReference<>();
137+
Sinks.One<Message<T>> sink = Sinks.one();
138+
139+
return sink.asMono()
140+
.doOnCancel(() -> scheduler.schedule(() -> {
141+
Message<?> undeliveredMessage = pendingMessage.getAndSet(null);
142+
handleUndeliveredMessage(messageSource, undeliveredMessage);
143+
}))
144+
.doOnRequest(request -> scheduler.schedule(() -> emitReceivedMessage(messageSource, pendingMessage, sink)));
145+
})
155146
.doOnSuccess((message) -> {
156147
if (message != null) {
157148
AckUtils.autoAck(StaticMessageHeaderAccessor.getAcknowledgmentCallback(message));
@@ -165,7 +156,7 @@ public static <T> Flux<Message<T>> messageSourceToFlux(MessageSource<T> messageS
165156
}
166157
LOGGER.error("Error from Flux for : " + messageSource, ex);
167158
})
168-
.subscribeOn(Schedulers.single())
159+
.subscribeOn(scheduler)
169160
.repeatWhenEmpty((repeat) ->
170161
repeat.flatMap((increment) ->
171162
Mono.deferContextual(ctx ->
@@ -204,6 +195,26 @@ else if (messageChannel instanceof PollableChannel pollableChannel) {
204195
}
205196
}
206197

198+
@SuppressWarnings("NullAway")
199+
private static <T> void emitReceivedMessage(MessageSource<T> messageSource,
200+
AtomicReference<Message<?>> pendingMessage, Sinks.One<Message<T>> sink) {
201+
202+
try {
203+
Message<T> message = messageSource.receive();
204+
if (message != null) {
205+
pendingMessage.set(message);
206+
}
207+
Sinks.EmitResult emitResult = sink.tryEmitValue(message);
208+
if (emitResult == Sinks.EmitResult.FAIL_CANCELLED || emitResult == Sinks.EmitResult.FAIL_TERMINATED) {
209+
handleUndeliveredMessage(messageSource, message);
210+
}
211+
pendingMessage.set(null);
212+
}
213+
catch (Exception ex) {
214+
sink.tryEmitError(ex);
215+
}
216+
}
217+
207218
private static void handleUndeliveredMessage(MessageSource<?> messageSource, @Nullable Message<?> message) {
208219
if (message == null) {
209220
return;
@@ -231,7 +242,11 @@ private static final class PollableChannelMessageSource<T> implements MessageSou
231242
}
232243

233244
void returnMessage(Message<?> message) {
234-
this.channel.send(message);
245+
if (!this.channel.send(message, 0)) {
246+
LOGGER.warn("Failed to return undelivered message to pollable channel [" + this.channel
247+
+ "]; nacking instead");
248+
AckUtils.autoNack(StaticMessageHeaderAccessor.getAcknowledgmentCallback(message));
249+
}
235250
}
236251

237252
}

spring-integration-core/src/test/java/org/springframework/integration/channel/reactive/ReactiveStreamsConsumerTests.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,49 @@ public void messageNotLostOnStopWithPollableChannel() throws InterruptedExceptio
291291
assertThat(stealCount).isZero();
292292
}
293293

294+
@Test
295+
@SuppressWarnings("unchecked")
296+
public void messageNotLostOnStopWithBoundedPollableChannel() throws InterruptedException {
297+
int iterations = 500;
298+
int stealCount = 0;
299+
for (int iteration = 0; iteration < iterations; iteration++) {
300+
QueueChannel testChannel = new QueueChannel(1);
301+
Subscriber<Message<?>> testSubscriber = (Subscriber<Message<?>>) Mockito.mock(Subscriber.class);
302+
BlockingQueue<Message<?>> messages = new LinkedBlockingQueue<>();
303+
304+
willAnswer(i -> {
305+
messages.put((Message<?>) i.getArgument(0));
306+
return null;
307+
}).given(testSubscriber).onNext(any(Message.class));
308+
309+
ReactiveStreamsConsumer reactiveConsumer = new ReactiveStreamsConsumer(testChannel, testSubscriber);
310+
reactiveConsumer.setBeanFactory(TEST_INTEGRATION_CONTEXT);
311+
reactiveConsumer.afterPropertiesSet();
312+
reactiveConsumer.start();
313+
314+
Message<?> testMessage = new GenericMessage<>("test");
315+
testChannel.send(testMessage);
316+
317+
ArgumentCaptor<Subscription> captor = ArgumentCaptor.forClass(Subscription.class);
318+
verify(testSubscriber).onSubscribe(captor.capture());
319+
captor.getValue().request(1);
320+
321+
assertThat(messages.poll(10, TimeUnit.SECONDS)).isSameAs(testMessage);
322+
323+
reactiveConsumer.stop();
324+
testChannel.send(new GenericMessage<>("bait-" + iteration));
325+
326+
for (int spin = 0; spin < 20000 && testChannel.getQueueSize() > 0; spin++) {
327+
// busy-poll to hit the race window
328+
}
329+
330+
if (testChannel.getQueueSize() == 0) {
331+
stealCount++;
332+
}
333+
}
334+
assertThat(stealCount).isZero();
335+
}
336+
294337
@Test
295338
public void testReactiveStreamsConsumerViaConsumerEndpointFactoryBean() throws Exception {
296339
FluxMessageChannel testChannel = new FluxMessageChannel();

0 commit comments

Comments
 (0)