[jira] [Commented] (CAMEL-24074) camel-jms: TemporaryQueueReplyManager resolves the temporary reply queue without synchronization - replies can be black-holed (CAMEL-20769 follow-up)

["Claus Ibsen (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CAMEL-24074?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18096536#comment-18096536 ] 

Claus Ibsen commented on CAMEL-24074:
-------------------------------------

Merged via https://github.com/apache/camel/pull/24731

> camel-jms: TemporaryQueueReplyManager resolves the temporary reply queue without synchronization - replies can be black-holed (CAMEL-20769 follow-up)
> -----------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: CAMEL-24074
>                 URL: https://issues.apache.org/jira/browse/CAMEL-24074
>             Project: Camel
>          Issue Type: Bug
>          Components: camel-jms
>    Affects Versions: 4.21.0
>            Reporter: Federico Mariani
>            Assignee: Claus Ibsen
>            Priority: Major
>              Labels: code-review
>             Fix For: 4.22.0
>
>         Attachments: TemporaryQueueReplyManagerConcurrentResolveTest.java
>
>
> h3. Problem
> {{TemporaryQueueReplyManager.TemporaryReplyQueueDestinationResolver.resolveDestinationName}} (TemporaryQueueReplyManager.java:285-315) is a check-then-act on the plain, non-volatile {{queue}} field with no synchronization. CAMEL-20769 (7313a1e53a63, May 2024) deliberately removed the previous {{synchronized (refreshWanted)}} block to fix an endless lock during shutdown - but replaced it with nothing.
> With {{replyToConcurrentConsumers > 1}} (a supported configuration, see {{JmsRequestReplyTempQueueMultipleConsumersTest}}), several listener-container invoker threads resolve the destination concurrently at startup and after every {{scheduleRefresh()}}:
> * T1 sees {{queue==null}}, creates Q1; T2 concurrently creates Q2, writes {{queue=Q2}}, calls {{setReplyTo(Q2)}}; T1's {{setReplyTo(Q1)}} lands last -> {{replyTo=Q1}} while consumers listen on Q2.
> * Once T1's cached consumer on Q1 is recycled, every reply is sent to Q1 and *black-holed* -> all InOut requests time out until a JMS exception forces a refresh.
> * A mid-refresh {{scheduleRefresh()}} can also be swallowed ({{refreshWanted.set(false)}} before the new queue is fully published).
> h3. Reproducer
> Attached {{TemporaryQueueReplyManagerConcurrentResolveTest.java}} (place in {{components/camel-jms/src/test/java/org/apache/camel/component/jms/reply/}}). It drives the resolver from two threads with a barrier inside {{createTemporaryQueue()}} making the race deterministic: today two temporary queues are created (the test expects one). A correctly synchronized resolver passes - the barrier simply times out for the single caller. No broker needed.
> h3. Note on the fix
> The CAMEL-20769 shutdown-deadlock motivation must be preserved - synchronize the resolver on its own dedicated lock (the same pattern QueueReplyManager's DestinationResolverDelegate uses, with the in-code explanation of why not BaseService.lock).
> ----
> _This issue was researched and filed by Claude Code on behalf of [~croway] (GitHub: Croway), as part of a deep code review of camel-jms and camel-amqp. A failing JUnit reproducer is attached; it fails deterministically on current main (4.22.0-SNAPSHOT)._



--
This message was sent by Atlassian Jira
(v8.20.10#820010)