.log() should always behave the same, no matter where it is placed in a flow

[swiss-chris]

Expected Behavior

Given some code like this:

        .scatterGather(
                scatterer -> scatterer
                        .applySequence(true)
                        .recipientFlow(IntegrationFlows.from(MessageChannels.executor(myExecutorService))
                                .gateway(mySubFlow1())
                                .get())
                        .recipientFlow(IntegrationFlows.from(MessageChannels.executor(myExecutorService))
                                .log(/* some logging here */)
                                .gateway(mySubFlow2())
                                .get()),
                gatherer -> gatherer
                        .releaseStrategy(defaultReleaseStrategy)
                        .outputProcessor(myOutputProcessor))

if I move the .log() statement down one line, like this:

        .scatterGather(
                scatterer -> scatterer
                        .applySequence(true)
                        .recipientFlow(IntegrationFlows.from(MessageChannels.executor(myExecutorService))
                                .gateway(mySubFlow1())
                                .get())
                        .recipientFlow(IntegrationFlows.from(MessageChannels.executor(myExecutorService))
                                .gateway(mySubFlow2())
                                .log(/* some logging here */)
                                .get()),
                gatherer -> gatherer
                        .releaseStrategy(defaultReleaseStrategy)
                        .outputProcessor(myOutputProcessor))

I would expect everything to work exactly the same, except that the .log() statement is executed after the .gateway() call.

Current Behavior

Changing the position of .log() changes the behavior of the flow. In this case, the releaseStrategy never receives the output of the second recipientFlow. The reason for this is explained in the JavaDoc, where we read - "When this operator is used in the end of flow, it is treated as one-way handler without any replies to continue.".

Context

This is not the behavior one would expect, it's not intuitive, and it has caused an unexpected bug in our team that took a long time to debug. (https://en.m.wikipedia.org/wiki/Principle_of_least_astonishment) We were quite surprised to learn about .logAndReply() and consider it one of those things that make the SI DSL hard to work with. Sometimes things just don't work and you have not reason why and no easy and no quick way to find the cause.

IMO the .log() statement should behave the same no matter where you place it in a flow. If it is placed at the end of a flow, and if this flow has no output channel, throw the usual exception, and the developer will quickly realize they need to add .nullChannel() or something similar. Perhaps you could even detect if .log() needs to reply or not (it seems you can detect if it is at the end of a flow, so maybe at that moment you can also detect if an output channel is configured for this flow) ?

logAndReply() could be deprecated and ultimately removed.

Am I missing something that would prevent such a solution ?

[artembilan]

Thank you for your report and such a great problem explanation, @swiss-chris !

We definitely keep our users in mind when we develop an API like this Java DSL, but indeed I missed that principle from the start.
As you noticed an unexpected behavior you spotted only after reading JavaDocs and hours of debugging.
So, that JavaDoc'ing still don't help.

Saying that I would like to share with you our journey how we came up with logAndReply() and why log() in the end is different.
See a related PR and its hard discussion including mentioned in the header JIRA ticket: #2535.

So, you are not the first who bumped his/her head against this log() wall in the end of flow, therefore I agree that we need to revise it.

Here is my suggestion (which is not different from yours):

1. If log() in the end of flow, add implicit bridge() to align replying behavior for log() how it is used in the middle of the flow.
2. It is probably going to be the most use-cases when people place log() in the end of flow, so they really expect replying back to the caller as it would work even without log().
3. If you want to stop the flow at this point, the nullChannel() is indeed the way to go. Exactly same behavior if you would remove that log() from your flow definition.
4. Deprecate (and remove eventually) logAndReply() since its behavior will be included into a log().
5. If end-user will encounter "no output-channel or replyChannel header available" at this point, they would get it even without log() anyway.

In general we are pursuing the same behavior as it would be even without log(), so logically not a breaking change from the high perspective, but still behavior change for current log() in the end of flow. So, we can do this only in the next 6.0 version starting this fall.

Any further comments are welcome!

(Sorry for late reply: was on vacation)

[swiss-chris]

Hi Artem
Thank you for this ! I'm happy to hear my proposal makes sense and look forward to the update ! We've been using the SI DSL a lot lately in our projects and will love every improvement.

[artembilan]

@garyrussell ,

I also need your opinion on this and if we are in an agreement, WDYT about deprecating logAndReply() just right now, so we can remove it easily in 6.0 ?

Thanks

[garyrussell]

Yes; it sounds reasonable, but not clear how you can deprecate, unless you say deprecated in favor of .log().bridge() in this release and the .bridge() becomes unnecessary in 6.0 (and you'd end up with - logically - .log().bridge().bridge() until it's removed.

[artembilan]

If it is an explicit .log().bridge(), then we are not going to add our internal .bridge(). Just need to be smart enough 😄
Although I see your point: let's don't make people to move away from logAndReply() right now since they will forget to fix it in the future!
When we realign log() behavior, we will deprecate logAndReply(), so end-user would need to do only one refactoring in their code.

Taking back my deprecation suggestion for the current version even if we will live with it in 6.x for some time.
Sorry for a noise

[swiss-chris]

@artembilan Is this still scheduled for inclusion in 6.0 ?

[artembilan]

Same here: it is in plan for the current 6.0.
Again, contribution is welcome: The contribution is welcome though: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc !