Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DATAGO-68275: fix SolaceErrorMessageHandler acknowledgmentCallback detection and error handling #331

Conversation

Nephery
Copy link
Collaborator

@Nephery Nephery commented Aug 28, 2024

This prevents an edge case of messages being stuck in an unacknowledged state when a MessagingException is thrown which contains a failedMessage that doesn't have its acknowledgmentCallback header set.

e.g.:

@Bean
public Consumer<Message<?>> sink(){
    return msg -> throw new MessagingException(MessageBuilder.fromMessage(msg)
			.removeHeader(IntegrationMessageHeaderAccessor.ACKNOWLEDGMENT_CALLBACK)
			.build());
}

Issue 1: Detection of AcknowledgmentCallback

Currently, this binder's error message handler detects the message's acknowledgmentCallback in the following order of precedence, where upon first discovery, the other potential sources for this callback are not searched:

  1. From the errorMessage's header.
    • Injected here by this binder's consumer binding if an exception was thrown during conversion of the SMF message to the Spring Message<?>.
  2. If the thrown exception is a MessagingException that has a failedMessage set, then get it from that failedMessage.
    • what is causing the above issue.
  3. the original message.
    • The original Spring Message<?> created by this binder's consumer binding.

Fix: Detect and handle ack callback from all sources

Detect and handle acknowledgmentCallback from all potential sources.

NOTE: Ideally, we should just ignore source 2 (failedMessage message set on the MessagingException), but I wasn't sure if this would cause backwards incompatibility. @mgevantmakher @carolmorneau , if you think that this isn't a use case and I'm worrying for nothing, then I can just remove that source entirely...

Issue 2: When the error message handler needs to fail

Currently, the error message handler just returns when it isn't able to process an error. This is faulty, since Spring will treat the errored message as "successfully handled", and returns control back to the binder as-if the message was processed successfully.

This results in the message getting acknowledged, and is the other contributor to the above issue.

Fix: Error handler should throw an exception when it isn't able to ack the message

Just throwing an exception instead of returning...

Copy link
Collaborator

@carolmorneau carolmorneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should just ignore source 2

I'm happy to follow your lead on this if you feel like it is needed. General sentiment when it comes to backward compatibility: "better safe than sorry".

@Nephery Nephery merged commit b64f11d into SolaceProducts:master Aug 29, 2024
2 checks passed
@Nephery Nephery deleted the fix-error-msg-handler-ack-callback-discovery branch August 29, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants