GH-10163: Fix async mode dropping errors #10183

rrileyca · 2025-07-01T15:53:17Z

A clean branch fixing several issues from my "corrupt" previous PR #10164

This PR has the RabbitStreamMessageHandler fallback to a sendFailureChannelName of "errorChannel" when a channel name is not set and when it is operating in async mode.

…endFailureChannelName Signed-off-by: rrileyca <[email protected]>

rrileyca · 2025-07-01T15:53:45Z

@artembilan here is the clean branch. Sorry again for the confusion.

artembilan · 2025-07-01T16:29:58Z

A clean branch fixing several issues from my "corrupt" previous PR

Why don't you create a top-level branch as I suggested in our previous discussion?
Why make your life so hard trying to contribute from your main?
The rule of thumb is to never develop in main.
See more info in our Contribution Guidelines: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc

rrileyca · 2025-07-01T16:36:52Z

I must admit I don't do much fork style PRs. I will do that in future. Thanks for the advice.

Do you want me to switch to another branch still?

artembilan

Please, add your name to the @author list of the affected classes.

artembilan · 2025-07-01T16:39:06Z

.../src/main/java/org/springframework/integration/amqp/outbound/RabbitStreamMessageHandler.java

+	protected String getSendFailureChannelNameOrDefault() {
+		return this.sendFailureChannelName == null ?
+				IntegrationContextUtils.ERROR_CHANNEL_BEAN_NAME :
+				sendFailureChannelName;


I think this should do that trick instead of your extra method calls:

protected @Nullable MessageChannel getSendFailureChannel() { if (this.sendFailureChannel == null && (this.sendFailureChannelName != null || !this.sync)) { String sendFailureChannelNameToUse = this.sendFailureChannelName; if (sendFailureChannelNameToUse == null) { sendFailureChannelNameToUse = IntegrationContextUtils.ERROR_CHANNEL_BEAN_NAME; } this.sendFailureChannel = getChannelResolver().resolveDestination(sendFailureChannelNameToUse); } return this.sendFailureChannel; }

I will slightly modify this, as it doesn't allow for sync mode with a configured sendFailureChannelName - unless that is your intention that in sync mode, a sendFailureChannel shall never be used.

I'm not sure what you are talking about.
See this condition:

this.sendFailureChannelName != null || !this.sync

So, we go down in a sync mode and provided name.
Then we do if (sendFailureChannelNameToUse == null) {.
Which applies would not be true in sync mode and provided name.
What do I miss?

You are right, I misread this :)

Good. So, please, incorporate whatever I've suggestion.
So, far your code is not what we have discussed.

artembilan · 2025-07-01T16:44:18Z

...test/java/org/springframework/integration/amqp/outbound/RabbitStreamMessageHandlerTests.java

+		assertThat(errorMessage).isNotNull();
+		Throwable caughtException = errorMessage.getPayload();
+		assertThat(caughtException).isInstanceOf(StreamSendException.class);
+		assertThat(caughtException).isEqualTo(streamException);


You can combine all of these into a single chained assert:

assertThat(errorMessage).extracting(Message::getPayload).isEqualTo(streamException);

I don't see a reason in extra isInstanceOf() since that isEqualTo() does cover this as well.

artembilan · 2025-07-01T16:45:09Z

...test/java/org/springframework/integration/amqp/outbound/RabbitStreamMessageHandlerTests.java

+						.build())
+				.build();
+		assertThatThrownBy(() -> handler.handleMessage(testMessage))
+				.isInstanceOf(MessageHandlingException.class);


We prefer to use opposite API: assertThatExceptionOfType()

artembilan · 2025-07-01T16:47:43Z

...test/java/org/springframework/integration/amqp/outbound/RabbitStreamMessageHandlerTests.java

+		RabbitStreamMessageHandler handler = RabbitStream.outboundStreamAdapter(streamTemplate)
+				.getObject();
+
+		String failureChannelName = handler.getSendFailureChannelNameOrDefault();


Exactly!
I don't want to have any protected methods just to satisfy tests.
You technically covered this code path with that errorChanelAsync().
Which indeed is kind of an integration test, but it also includes a failureChannel path.
Therefore I don't see a reason in extra tests.

artembilan · 2025-07-01T16:49:23Z

Do you want me to switch to another branch still?

It depends how far we still would go out of what we have agreed before 😉

Signed-off-by: rrileyca <[email protected]>

rrileyca · 2025-07-01T19:31:41Z

It depends how far we still would go out of what we have agreed before 😉

There was no intention to deviate on what we agreed on. This is my first time contributing to Spring, so I have a lot to learn. Thank you for your feedback and patience.

Signed-off-by: rrileyca <[email protected]>

artembilan

Please, see that contributing doc: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc#provide-a-link-to-the-github-issue-in-the-associated-pull-request.

We recommend to have a link to the issue like Fixes: exactly in the commit message.
Whatever you say on PR won't close issue automatically.

Plus, the first commit of the PR becomes very own PR title and description.
Therefore, here is a best practice for Git commits: https://cbea.ms/git-commit/.

And you already know that it is not very convenient to raise a PR against main.
Usually we pull your branch locally for final review and merge.
If that is done from main, then it causes some headache on merge, not only for your local project copy.

artembilan · 2025-07-01T20:12:07Z

Now, since we are very close to the merge, tell us, please, if you are going to contribute similar fix for the KafkaProducerMessageHandler.
We have that discussed in the issue and that one is scheduled for Kafka module fix as well.
I don't think we need dedicated tests in that module since the functionality is exactly the same you have done here.
I'm asking for the fix over there as well, since the issue is also scheduled for back-port to previous versions.

Thank you again!

artembilan

The build has failed:

 Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-amqp/src/test/java/org/springframework/integration/amqp/outbound/RabbitStreamMessageHandlerTests.java:43:59: Using a static member import should be avoided - org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType. [AvoidStaticImport]

Please, run ./gradlew :spring-integration-amqp:check locally before pushing.

rrileyca · 2025-07-01T20:18:36Z

tell us, please, if you are going to contribute similar fix for the KafkaProducerMessageHandler.

Yes, I can add that.

And you already know that it is not very convenient to raise a PR against main.

Apologies, I did not know. Does this mean I should create a new PR from the new branch?

artembilan · 2025-07-01T20:24:11Z

Does this mean I should create a new PR from the new branch?

No, it's fine so far.
Just would be some inconvenience if I would do clean up myself for you and manual merge locally.
But right now I had to ask you to do all of that boring stuff around the code perfection 😉 .

And then, when we merge, it would be a problem for your local main to bring those your changes back to your copy.
Right now you would need to do reset force or just remove the branch and fetch from upstream.

rrileyca · 2025-07-01T20:45:22Z

Well, I have to squash all the commits in order to make a proper first git commit, even if I keep this fork as is (with the incorrect commits on the main branch.

Plus, I hope to make more contributions in the future, so I think it is a good idea to just clean up and make a new fork, with the proper feature branch and properly written first commit.

Fix async mode dropping errors by falling back to "errorChannel" as s…

e25178a

…endFailureChannelName Signed-off-by: rrileyca <[email protected]>

rrileyca changed the title ~~Fix async mode dropping errors~~ GH-10163: Fix async mode dropping errors Jul 1, 2025

artembilan requested changes Jul 1, 2025

View reviewed changes

add author

173cf09

Signed-off-by: rrileyca <[email protected]>

rrileyca added 3 commits July 1, 2025 15:42

Refactor and remove useless protected method (used by tests only)

18fab22

Signed-off-by: rrileyca <[email protected]>

Fix tests to match Spring preferences

7475332

Signed-off-by: rrileyca <[email protected]>

Refactor getSendFailureChannel

f1d619f

Signed-off-by: rrileyca <[email protected]>

artembilan requested changes Jul 1, 2025

View reviewed changes

rrileyca closed this by deleting the head repository Jul 1, 2025

GH-10163: Fix async mode dropping errors #10183

GH-10163: Fix async mode dropping errors #10183

Conversation

rrileyca commented Jul 1, 2025

Uh oh!

rrileyca commented Jul 1, 2025

Uh oh!

artembilan commented Jul 1, 2025

Uh oh!

rrileyca commented Jul 1, 2025

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrileyca Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan commented Jul 1, 2025

Uh oh!

rrileyca commented Jul 1, 2025

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

artembilan commented Jul 1, 2025

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

rrileyca commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artembilan commented Jul 1, 2025

Uh oh!

rrileyca commented Jul 1, 2025

Uh oh!

Uh oh!

rrileyca Jul 1, 2025 •

edited

Loading

rrileyca commented Jul 1, 2025 •

edited

Loading