You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "allthingssecurity (via GitHub)" <gi...@apache.org> on 2026/10/14 00:05:35 UTC
[PR] CAMEL-25612: camel-sjms - a failed connection recovery attempt leaves the consumer without JMS consumers [camel]
allthingssecurity opened a new pull request, #27799:
URL: https://github.com/apache/camel/pull/27799
# Description
[CAMEL-25612](https://issues.apache.org/jira/browse/CAMEL-25612)
`SimpleMessageListenerContainer.initConsumers` assigned the sets of sessions and consumers before it filled them. When `createSession` or `createMessageConsumer` threw, the sets stayed, empty or half filled.
During connection recovery that was permanent:
1. attempt n creates the new connection, assigns the sets, one create call fails, the attempt fails (WARN) and is repeated after `recoveryInterval`;
2. attempt n+1 closes that connection (and the sessions of attempt n with it), creates another one, finds `consumers != null`, creates nothing, and reports success.
The route stayed started with a started connection and no consumer. Nothing above DEBUG was logged after the WARN of attempt n, no further recovery was scheduled, and no message was consumed until the route or the context was restarted. The reply listeners of the InOut producer use the same container.
A create call fails on a fresh connection when the connection fails again while the consumers are created (broker still restarting, failover) and `onException` of that failure arrives before `initConsumers` assigns the sets, or when the connection is fine and the destination or the permission is not there yet (then no `onException` follows, and every such failure ends this way).
Changes, all in `SimpleMessageListenerContainer`:
- `initConsumers` closes what it created and sets the two fields back to `null` when the loop fails, so the next attempt creates all the consumers.
- The same `catch` block closes the session whose consumer could not be created when the container fails to start. That session stayed open on the connection, which is kept for the next start.
- `recoverConnection` runs another attempt (it returns `false`, so the task runs again after `recoveryInterval`) when `onException` dropped the consumers while the attempt was creating them. `onException` does not schedule a recovery while one is running. On main this case mostly worked by accident: the loop added to the fields that `onException` had set to `null`, and the `NullPointerException` failed the attempt. The loop no longer fails that way, so the check is needed. Only an `onException` after the last pass of the loop was lost on main; that window is closed too.
No new retry loop and no new log per attempt: the attempts are those of the existing recovery task, one WARN line without a stack trace per failed attempt as before; the new case logs at DEBUG.
No upgrade guide entry: nothing a route can rely on changes.
Tests:
- New `SjmsConsumerRecoveryFailedAttemptTest` (4 tests) with the embedded Artemis broker of the module and a connection factory that tracks the connections, sessions and consumers of the route and can make one create call fail. `concurrentConsumers=3`, `recoveryInterval=200`.
- After a simulated connection failure the first session (one test) or the last session (another test) of the next connection cannot be created. The test waits for the attempt after that (the third connection), sends a message and expects it, then expects 3 consumers on the third connection.
- A container that fails to start because its second consumer cannot be created leaves no open session and no open consumer; a second start creates both consumers on the same connection.
- `onException` during the creation of the last session of the recovery connection: another attempt runs and the consumers are on a third connection.
- Without the change (main's `SimpleMessageListenerContainer`, same test) three of the four fail: `mock://first Received message count. Expected: <1> but was: <0>`, `mock://last Received message count. Expected: <1> but was: <0>`, and `open sessions after the failed start ==> expected: <0> but was: <1>`. The fourth one (`onException` during the creation) passes on main, through the `NullPointerException` described above; it is there because the loop no longer fails that way.
- With the change, camel-sjms tests pass: 138 run, 0 failures, 5 skipped.
What was not run: camel-sjms2 (it uses the same container), and no broker other than the embedded Artemis. The failing create call is simulated by the connection factory of the test.
Notes:
- The open pull request #27173 (CAMEL-16039) changes `onException` in the same file: it moves the three lines that reset the fields into a method. It does not touch `initConsumers` or `recoverConnection`, and the two changes merge without a conflict today.
- How it was found: a small TLA+ model of the container (connection alive or failed, the pending `onException` call, the `consumers` field, the consumers on the current connection, the steps of one recovery attempt). For main it reaches a state with a live connection, no consumer and no recovery scheduled, after two connection failures or after one connection failure and one failing create call. The model is only a sketch of the code; the test above is the evidence.
# Target
- [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch)
# Tracking
- [ ] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
(The ticket is drafted and not filed yet; CAMEL-25612 is a placeholder until it is.)
# Apache Camel coding standards and style
- [x] I checked that each commit in the pull request has a meaningful subject line and body.
- [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes.
(I built and tested camel-sjms, including the formatter and import-sort plugins. I did not run the full root build.)
# AI-assisted contributions
- [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used.
This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer.
_Claude Code on behalf of allthingssecurity_
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25612: camel-sjms - a failed connection recovery attempt leaves the consumer without JMS consumers [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27799:
URL: https://github.com/apache/camel/pull/27799#issuecomment-6160924081
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25612: camel-sjms - a failed connection recovery attempt leaves the consumer without JMS consumers [camel]
Posted by "allthingssecurity (via GitHub)" <gi...@apache.org>.
allthingssecurity commented on code in PR #27799:
URL: https://github.com/apache/camel/pull/27799#discussion_r4267168694
##########
components/camel-sjms/src/main/java/org/apache/camel/component/sjms/consumer/SimpleMessageListenerContainer.java:
##########
@@ -358,14 +365,31 @@ protected void initConsumers() throws Exception {
if (consumers == null) {
LOG.debug("Initializing {} concurrent consumers as JMS listener on destination: {}", concurrentConsumers,
destinationName);
- sessions = new HashSet<>(concurrentConsumers);
- consumers = new HashSet<>(concurrentConsumers);
- for (int i = 0; i < this.concurrentConsumers; i++) {
- Session session = createSession(connection, endpoint);
- MessageConsumer consumer = createMessageConsumer(session);
- configureConsumer(consumer, session);
- sessions.add(session);
- consumers.add(consumer);
+ Set<Session> newSessions = new HashSet<>(concurrentConsumers);
+ Set<MessageConsumer> newConsumers = new HashSet<>(concurrentConsumers);
+ sessions = newSessions;
+ consumers = newConsumers;
Review Comment:
Thanks, I read the fix again against this and it holds: `onException` nulls both fields under `connectionLock`, and
the check in `recoverConnection` reads `consumers` under the same lock after `initConsumers` returns, so a null set
during the loop is seen and the attempt returns `false`. If the loop fails, the `catch` nulls the fields itself and
the exception reaches `recoverConnection`. Done in 902cc2b5d447: your comment is above the two assignments, as suggested. No code change. camel-sjms: 138 run, 0 failures, 5 skipped.
On point 2 of the review (an `onException` for the new connection before `initConsumers` assigns the sets): agreed
that the signal is lost there if the provider creates sessions and consumers without noticing. I would do the failure
counter as a follow-up, with its own test, rather than in this approved PR.
_Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25612: camel-sjms - a failed connection recovery attempt leaves the consumer without JMS consumers [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27799:
URL: https://github.com/apache/camel/pull/27799#issuecomment-6161826285
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `components/camel-sjms`
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 10 of 703 tested, 27 compile-only — current: 10 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **10 affected modules** (current approach: 10).
**Skip-tests mode would test 10 modules** (1 direct + 9 downstream), **skip tests for 27** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (10)</summary>
- `camel-jbang-mcp` ← downstream of org.apache.camel:camel-catalog
- `camel-jbang-plugin-mcp` ← downstream of org.apache.camel:camel-jbang-core
- `camel-jbang-plugin-tui` ← downstream of org.apache.camel:camel-catalog
- `camel-jbang-plugin-validate` ← downstream of org.apache.camel:camel-yaml-dsl-validator
- `camel-launcher-container` ← downstream of org.apache.camel:camel-launcher
- `camel-mllp` ← downstream of org.apache.camel:camel-sjms
- `camel-sjms` ← components/camel-sjms/src/main/java/org/apache/camel/component/sjms/consumer/SimpleMessageListenerContainer.java, components/camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/SjmsConsumerRecoveryFailedAttemptTest.java
- `camel-sjms2` ← downstream of org.apache.camel:camel-sjms
- `camel-yaml-dsl-validator` ← downstream of org.apache.camel:camel-catalog
- `camel-yaml-dsl-validator-maven-plugin` ← downstream of org.apache.camel:camel-yaml-dsl-validator
</details>
<details><summary>Modules with tests skipped (27)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
<details><summary>All tested modules (37 modules, 7m 1s total)</summary>
**Total reactor time:** 7m 1s
| Module | Duration | Status |
| --- | --- | --- |
| Camel :: MLLP | 59.0s | SUCCESS |
| Camel :: Simple JMS | 56.6s | SUCCESS |
| Camel :: Launcher | 45.4s | SUCCESS |
| Camel :: JBang :: MCP | 36.7s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 23.2s | SUCCESS |
| Camel :: YAML DSL :: Validator | 22.9s | SUCCESS |
| Camel :: YAML DSL | 19.9s | SUCCESS |
| Camel :: JBang :: Plugin :: Kubernetes | 18.8s | SUCCESS |
| Camel :: Component DSL | 18.5s | SUCCESS |
| Camel :: JBang :: Plugin :: Validate | 16.5s | SUCCESS |
| Camel :: Docs | 14.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Testing | 11.6s | SUCCESS |
| Camel :: Kamelet Main | 11.1s | SUCCESS |
| Camel :: Simple JMS2 | 8.6s | SUCCESS |
| Camel :: YAML DSL :: Deserializers | 7.8s | SUCCESS |
| Camel :: Catalog :: Camel Route Parser | 7.6s | SUCCESS |
| Camel :: Catalog :: Camel Report Maven Plugin | 7.3s | SUCCESS |
| Camel :: YAML DSL :: Validator Maven Plugin | 6.9s | SUCCESS |
| Camel :: All Components Sync point | 5.4s | SUCCESS |
| Camel :: Catalog :: Maven | 3.4s | SUCCESS |
| Camel :: YAML DSL :: Maven Plugins | 3.1s | SUCCESS |
| Camel :: Catalog :: Suggest (deprecated) | 2.3s | SUCCESS |
| Camel :: Assembly | 2.2s | SUCCESS |
| Camel :: JBang :: Integration tests | 1.8s | SUCCESS |
| Camel :: JBang :: Plugin :: MCP | 1.4s | SUCCESS |
| Camel :: Coverage | 1.3s | SUCCESS |
| Camel :: JBang :: Plugin :: Edit | 1.3s | SUCCESS |
| Camel :: Catalog :: Dummy Component | 1.1s | SUCCESS |
| Camel :: Catalog :: Console | 1.0s | SUCCESS |
| Camel :: JBang :: Plugin :: Generate | 1.0s | SUCCESS |
| Camel :: JBang :: Main | 0.8s | SUCCESS |
| Camel :: Endpoint DSL :: Support | 0.8s | SUCCESS |
| Camel :: Launcher :: Container | 0.7s | SUCCESS |
| Camel :: Endpoint DSL | n/a | |
| Camel :: Integration Tests | n/a | |
| Camel :: JBang :: Core | n/a | |
| Camel :: JBang :: Plugin :: TUI | n/a | |
**Top 20 slowest modules:**
- `Camel :: MLLP` (59.0s)
- `Camel :: Simple JMS` (56.6s)
- `Camel :: Launcher` (45.4s)
- `Camel :: JBang :: MCP` (36.7s)
- `Camel :: Catalog :: Camel Catalog` (23.2s)
- `Camel :: YAML DSL :: Validator` (22.9s)
- `Camel :: YAML DSL` (19.9s)
- `Camel :: JBang :: Plugin :: Kubernetes` (18.8s)
- `Camel :: Component DSL` (18.5s)
- `Camel :: JBang :: Plugin :: Validate` (16.5s)
- `Camel :: Docs` (14.8s)
- `Camel :: JBang :: Plugin :: Testing` (11.6s)
- `Camel :: Kamelet Main` (11.1s)
- `Camel :: Simple JMS2` (8.6s)
- `Camel :: YAML DSL :: Deserializers` (7.8s)
- `Camel :: Catalog :: Camel Route Parser` (7.6s)
- `Camel :: Catalog :: Camel Report Maven Plugin` (7.3s)
- `Camel :: YAML DSL :: Validator Maven Plugin` (6.9s)
- `Camel :: All Components Sync point` (5.4s)
- `Camel :: Catalog :: Maven` (3.4s)
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/38488147277)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25612: camel-sjms - a failed connection recovery attempt leaves the consumer without JMS consumers [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #27799:
URL: https://github.com/apache/camel/pull/27799#discussion_r4266930738
##########
components/camel-sjms/src/main/java/org/apache/camel/component/sjms/consumer/SimpleMessageListenerContainer.java:
##########
@@ -358,14 +365,31 @@ protected void initConsumers() throws Exception {
if (consumers == null) {
LOG.debug("Initializing {} concurrent consumers as JMS listener on destination: {}", concurrentConsumers,
destinationName);
- sessions = new HashSet<>(concurrentConsumers);
- consumers = new HashSet<>(concurrentConsumers);
- for (int i = 0; i < this.concurrentConsumers; i++) {
- Session session = createSession(connection, endpoint);
- MessageConsumer consumer = createMessageConsumer(session);
- configureConsumer(consumer, session);
- sessions.add(session);
- consumers.add(consumer);
+ Set<Session> newSessions = new HashSet<>(concurrentConsumers);
+ Set<MessageConsumer> newConsumers = new HashSet<>(concurrentConsumers);
+ sessions = newSessions;
+ consumers = newConsumers;
Review Comment:
These two assignments before the loop are what let `recoverConnection` see an `onException` that arrives while the loop runs (it nulls the fields, and the check at line 215 then retries). That is not obvious from here. Suggest a short comment, for example:
```suggestion
// assigned before the loop, so that an onException during the creation (it sets the fields to null)
// is seen by recoverConnection, which then tries again
sessions = newSessions;
consumers = newConsumers;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25612: camel-sjms - a failed connection recovery attempt leaves the consumer without JMS consumers [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #27799:
URL: https://github.com/apache/camel/pull/27799
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org