allthingssecurity opened a new pull request, #29120:
URL: https://github.com/apache/camel/pull/29120
## CAMEL-26536: camel-jms - a consumer stopped and started again with
asyncStopListener=true must keep a running listener container
Found with a TLA+ model of `JmsConsumer` (the lifecycle thread, the
asynchronous stop task and the asynchronous start task on the component's
pool), cross-checked with a SPIN model, then reproduced on the real classes.
### The problem
With `asyncStopListener=true`, `JmsConsumer.doStop` submits
`stopAndDestroyListenerContainer()` to the component's thread pool and returns
with `listenerContainer` still set. When the same consumer is started again
before that task runs, `doStart` sees the old container, does not create a new
one, and initializes and starts it again. The stop task then reads the field,
stops and destroys that container, and sets the field to null. The consumer
reports started but has no listener container and consumes nothing, with
nothing logged.
```
1 lifecycle : doStop -> listenerContainer = C1, submit the stop task
(JmsConsumer:216-219)
2 lifecycle : doStart -> listenerContainer != null, keep C1
(:149)
3 lifecycle : C1.afterPropertiesSet(); C1.start(); initialized = true
(:177-189)
4 stop task : listenerContainer.stop(); listenerContainer.destroy()
(:195-196)
5 stop task : listenerContainer = null; initialized = false
(:203-205)
-> consumer started, no listener container
```
If the task nulls the field while `doStart` is between its null check and
`afterPropertiesSet()`, the start fails with a `NullPointerException`. With
`asyncStartListener=true` as well, the start task uses whatever the field holds
when it runs.
The same consumer instance is stopped and started by the route policies that
call `stopConsumer` / `startConsumer` (the leader-election policies
`HazelcastRoutePolicy` and `MasterRoutePolicy` of camel-zookeeper-master), by
the JMX start/stop operations of the consumer, and by code that calls `stop()`
/ `start()` on it. A route restart is not affected, as the route creates a new
consumer when it starts again.
### The fix
- `doStop` detaches the listener container under a lock of the consumer: the
container, its private thread pool and the `initialized` flag are taken and
cleared at once, and the (possibly asynchronous) stop stops and destroys only
the container and pool it was given. A start that runs before the stop task
creates a new listener container, as it already did after a synchronous stop.
- `doStart` passes the container it created (or kept) to the start task. The
task does nothing if the consumer no longer uses that container, and after
starting it checks again under the lock: if the consumer was stopped meanwhile,
it stops and destroys the container itself (the stop may have run before the
start).
- The protected methods `prepareAndStartListenerContainer()`,
`stopAndDestroyListenerContainer()`, `testConnectionOnStartup()` and the public
`startListenerContainer()` keep their signatures and work on the current
container, but `doStart` / `doStop` now call private variants that take the
captured container, so a subclass that overrides them is no longer called from
there. There is no subclass of `JmsConsumer` in Camel.
One small change for the synchronous stop: the consumer now drops its
container before stopping it, so if `stop()` or `destroy()` throws, a later
start creates a new container instead of reusing the failed one.
With the change, the TLA+ model holds for stop/start/stop/start, suspend and
resume sequences with every combination of the two options, and the SPIN model
agrees.
### Tests
`JmsAsyncStopListenerRestartTest` (new) gives the component an executor that
holds the tasks the consumer submits, and runs them itself in a chosen order
(in-VM Artemis, no `Thread.sleep`):
- stop and start the consumer with the stop task pending: the listener
container must run and the next message must be consumed;
- the same with `asyncStartListener=true`, running the stop task before and
after the start task;
- stop, start and stop with the start task pending: no listener container
left running;
- the consumer is stopped (and its stop task runs) while the start task
initializes the container, through a hook in a custom container: the container
must not be left running.
All five fail on main and pass with the change. The last one guards the
check after the start: on main it fails earlier (the start task meets the
nulled field), and without that check it fails because the container is left
running. The class ran 20 times in one JVM (100 tests) with no failure.
Without hooks, 30 stop/start cycles of a consumer with the real pool ended
30 times without a running listener container on main (27 started without a
listener, 3 starts failed with the NPE) and 0 times with the change.
camel-jms: 483 tests, 0 failures, 11 skipped (in-VM Artemis), including the
async start/stop, auto-startup and restart tests.
### Not part of this
In the same model, two corners of `asyncStartListener=true` with suspend
remain: a consumer suspended before its start task has run is started by that
task, and a resume in that window submits a second start task that initializes
the same container twice. They need a decision on how suspend should treat a
pending asynchronous start, so they are left out.
camel-sjms's `SjmsConsumer` has the same `asyncStopListener` shape (its stop
task stops and shuts down the listener container that a start in the meantime
started again). I will open a follow-up JIRA for it, and one for the two
suspend corners above, together with the existing double
`onListenerContainerStarting()` when `resume()` runs before the first start
(the counter then never returns to 0, so `JmsEndpoint` refuses to stop).
camel-spring-rabbitmq has no asynchronous start/stop options.
Affected: main and the maintenance branches: 4.23.x and 4.22.x have the same
`JmsConsumer` code; 4.18.x and 4.14.x have the same bug, but set `initialized`
in `doStart`, so a backport needs a small adaptation.
_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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
