search

spring-cloud / spring-cloud-stream

Framework for building Event-Driven Microservices
http://cloud.spring.io/spring-cloud-stream
Apache License 2.0
1.01k stars 614 forks source link

Remove redundant `delaySubscription` from `FunctionConfiguration` #2978

Closed artembilan closed 3 months ago

artembilan commented 3 months ago

Related to: https://github.com/spring-projects/spring-integration/issues/9362

After the fix in Spring Integration: https://github.com/spring-projects/spring-integration/commit/bdcb856a9081bc091d56be16d51f0f4d561bc9ce we ended up in a deadlock situation with a beginPublishingTrigger in the FunctionConfiguration used for the delaySubscription() on an original Publisher. The FluxMessageChannel uses its own delaySubscription() until the channel has its subscribers. Apparently the logic before was with errors, so the FluxMessageChannel was marked as active even if its subscriber is not ready yet, leading to famous Dispatcher does not have subscribers error. So, looks like this beginPublishingTrigger was introduced back in days in Spring Cloud Stream to mitigate that situation until we really emit a BindingCreatedEvent.

The deadlock (and the flaw, respectively) is with the setupBindingTrigger() method implementation where FluxMessageChannel now "really" delays a subscription to the provided Publisher, therefore not triggering that Mono.create() fulfilment immediately. The BindingCreatedEvent arrives earlier, than we have a subscriber on the channel, but triggerRef.get() is null, so we don't success() it and in the end don't subscribe to an original Publisher since delaySubscription() on it is never completed.

Since FunctionConfiguration fully relies on IntegrationFlow.from(Publisher), which ends up with the mentioned FluxMessageChannel.subscribeTo() and its own delaySubscription() (which, in turn, apparently fixed now), we don't need our own delaySubscription() any more. Therefore the fix in this PR is to propose to remove beginPublishingTrigger logic altogether.

sobychacko commented 3 months ago

Merged via https://github.com/spring-cloud/spring-cloud-stream/commit/3b5bb2e0d9a89c3b9ae9152c99268460ca7ed442.

@olegz We need to discuss this when you are back. I merged it to address some CI test failures.

wrwksexahatvani commented 1 month ago

@artembilan @sobychacko Any update on the release date?

sobychacko commented 1 month ago

Hi, this will be part of the 2024.0.0-M2 release of spring-cloud scheduled for 10/03/24.

https://calendar.spring.io/