CAMEL-24146: Fix FluentProducerTemplate leak in InMemorySagaCoordinator#24830
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
LGTM — straightforward resource leak fix.
Every doFinalize() call created a new FluentProducerTemplate that was never stopped, leaking a ProducerTemplate with its producer cache on each invocation (including retries). The fix is clean:
-
Proper lifecycle — shared
ProducerTemplatecreated indoStart(), stopped indoStop(), following the same pattern asexecutorService. Null guard handles restart scenarios. -
Thread-safe —
ProducerTemplateis designed to be shared across threads, so reusing it across concurrent coordinator finalization calls is correct. -
Simplification — replacing
createFluentProducerTemplate().to(endpoint).withExchange(target).send()withproducerTemplate.send(endpoint, target)is a clean simplification — the fluent API added no value for this single-endpoint, exchange-based send. -
Consistent API —
getProducerTemplate()mirrors the existinggetExecutorService()pattern on the same class.
Minimal change, clear impact.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 550 tested, 29 compile-only — current: 550 all testedMaveniverse Scalpel detected 579 affected modules (current approach: 550).
|
oscerd
left a comment
There was a problem hiding this comment.
LGTM — clean leak fix. Creating a FluentProducerTemplate per finalization call leaked it (never closed); replacing it with a single lifecycle-managed ProducerTemplate created in doStart() and stopped+nulled in doStop() (mirroring the existing executorService pattern, null-guarded for restart) fixes that, and getProducerTemplate().send(endpoint, target) is semantically identical to the old .to(endpoint).withExchange(target).send(). ProducerTemplate is thread-safe for concurrent finalization. (No direct no-leak assertion, which is fair — hard to unit-test; covered indirectly by the existing retry-path test.)
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Summary
Claude Code on behalf of davsclaus
InMemorySagaCoordinator.doFinalizecreated a newFluentProducerTemplateper compensation/completion call (including retries) and never stopped it. Each leaked template holds a startedProducerTemplatewith a producer cache (default max 1000). With the default 5 retry attempts, a single saga finalization could leak up to N*5 templates.Fix: Create a shared
ProducerTemplateinInMemorySagaService(which already hasdoStart/doStoplifecycle) and reuse it across all coordinator finalization calls. TheFluentProducerTemplatewas overkill here — the only call was.to(endpoint).withExchange(target).send(), which delegates toProducerTemplate.send(Endpoint, Exchange).Changes:
InMemorySagaService: add aProducerTemplatefield, create indoStart(), stop indoStop()InMemorySagaCoordinator: replace per-callcreateFluentProducerTemplate()with the sharedsagaService.getProducerTemplate().send()Test plan
SagaTest,SagaFailuresTest,SagaOptionsTest,SagaPropagationTest,SagaRemoveHeadersTest,SagaTimeoutTest,SagaComponentTest)SagaFailuresTestexercises the retry code path where the leak was most severeCo-Authored-By: Claude Opus 4.6 noreply@anthropic.com