Hello Spring comunity,
I was recently in the process of migrating a project from Spring Boot 2.6.14 to 3.3.4.
In our project we are using Tibco EMS, and their custom ack mode: EXPLICIT_CLIENT_ACKNOWLEDGE (tibco doc), as we want to have control over each individual message.
After the migration I encountered the problem that during the acknowledgement exceptions were being thrown, while the messages were acknowledged.
The root of the problem was a new implementation of the isClientAcknowledgement method (added in this PR).
The problem flow is the following: read message -> manually acknowledge -> AbstractMessageListenerContainer tries to acknowledge the message a second time, which causes an exception to be thrown.
Link to the repo with the reproduced problem: link In the above repo the problem was reproduced on Embedded ActiveMQ (with ack mode INDIVIDUAL_ACKNOWLEDGE), but it looks the same for Tibco EMS as well
Comment From: jhoeller
What kind of exception is being thrown from the message.acknowledge() call in AbstractMessageListenerContainer in your scenario? A specific JMSException subtype on both Tibco and ActiveMQ? Or just a specific exception message?
In general, we do not recommend manual acknowledgement in a custom way in combination with Spring's message listener containers. Throwing an exception from the listener endpoint should do the right thing by default: recovering the JMS Session instead of acknowledging the Message. This is the only officially supported way to force redelivery of the current message.
I suppose you use manual acknowledgement as an alternative to that default behavior? Are you using it to acknowledge the message early before starting the subsequent processing in your listener implementation, in which case we could simply ignore an exception from our default acknowledge call after the listener returned? Or are you expecting a non-manually-acknowledged message to be redelivered despite the listener endpoint having returned normally, specifically skipping the message.acknowledge() call in your listener implementation for the purpose of forcing redelivery of the message?
In terms of potential solutions, we could suppress certain exceptions when the acknowledge mode is non-standard, while still defensively trying our default acknowledge call which is necessary for proper default processing in common listener endpoints (which do not call acknowledge themselves and often do not even see the JMS Message, just a payload object).
Alternatively, we could handle certain custom acknowledge modes on certain JMS provider specifically, in particular when the JMS Session needs to remain completely untouched - that is, with the Message not to be acknowledged at all and the Session not to be recovered either. However, that would be quite a semantic deviation in Spring's listener endpoint model.
Comment From: UladzislauBlok
Hey Juergen, 1. We do see the standart IllegalArgumetExeption, but thrown from tibco lib. For ActiveMQ, there is a ActiveMQIllegalStateException 2. I've checked our code one more time. The problem here not with the exeption only. We do processing in custom reactor 'pipeline' with a lot of calls to the external systems. To not block jms listener thread for huge amout of time we do schedule on the reactor scheduler thread. Message flow in a new thread is the next: validate message (external calls) -> ack in any case (validation is passed, message is send to jms / validation is failed do the same, but for another jms) in flux.doOnTermonate operator. We expect to have EOS, but in case of ack message by listener we could see the next situation: [listener-thread] read message -> [reactor-thread] validate it -> [listener-thread] ack message (thread was back to the listener container after publication to a scheduler) -> app goes down before send the response -> this lead us to situation when we ack request, but didn't send the response to the another jms.
Comment From: UladzislauBlok
Or if our processing was done, we ack the message in a reactor thread, but also in a listener thread. Our temporary solution was back to previous implementation of JmsAccessor#isClientAcknowledge, where we explicitly compare ack mode with client ack (this.ackMode == CLIENT_ACKNOWLEDGE)
Comment From: jhoeller
Interesting, so that's in combination with a Reactor pipeline in the listener endpoint. I did not expect that, thanks for clarifying.
So you effectively accept some potential for losing a message by acknowleding it before it is fully processed? That's not the norm but fine if your system state can deal with it.
Do I understand correctly that you are not trying to force redelivery through not acknowleding at all, rather just early acknowledging in your listener processing workflow? In that case, our AbstractMessageListenerContainer could simply silently suppress such an exception from its message.acknowledge() attempt and move on (assuming that the listener endpoint has acknowledged the message already in that case). Would that help for your scenario?
Comment From: UladzislauBlok
Not really. We're never lose message because we do validation in our pipeline (in reactor thread), and ack message manual at the end (after even expection hadling).
Comment From: UladzislauBlok
We can lose messages in case of AbstractMessageListenerContainer acknowledgment, because the listener thread will be free before the reactor thread will be done with processing
Comment From: UladzislauBlok
I've prepare diagram to make flow more visible
Comment From: UladzislauBlok
So listener thread can ack message too early, and if appliaction goes down after that, but before reactor thread processing, we'll lose the message
Comment From: jhoeller
Ah ok, understood. That's the key difference to our regular processing scenario within the original listener thread: It does not hurt to do message.acknowledge() in AbstractMessageListenerContainer in the regular case since the listener ended processing anyway. Coming with the benefit that user code does not have to do message.acknowledge() itself at all even for vendor-specific acknowledge modes, enabling the use of those modes for efficiency reasons with the usual simple listener endpoint implementation style and no need for further customizations.
I'll reconsider what we can do to make your scenario work in a more obvious way. Simply suppressing message.acknowledge() exceptions in AbstractMessageListenerContainer won't be sufficient then, we really need to skip that container-level acknowledge() call in such scenarios, putting the onus entirely on the listener implementation. That's not ideal as default behavior (which we tried to address with that change in 6.0) - but for scenarios like yours, this needs to be easy to opt into.
For the time being, your workaround to tighten the isClientAcknowledge() implementation makes sense. This is all that's necessary to restore our previous behavior.
Comment From: UladzislauBlok
Proposal that we've discussed with team is probably introduce feature flag to disable container-level ack (enable this by default)