gnodet-bot commented on code in PR #29120:
URL: https://github.com/apache/camel/pull/29120#discussion_r4462084796
##########
components/camel-jms/src/main/java/org/apache/camel/component/jms/JmsConsumer.java:
##########
@@ -149,91 +161,145 @@ protected void doStart() throws Exception {
if (listenerContainer == null) {
createMessageListenerContainer();
}
+ final AbstractMessageListenerContainer container = listenerContainer;
getEndpoint().onListenerContainerStarting();
if (getEndpoint().getConfiguration().isAsyncStartListener()) {
getEndpoint().getAsyncStartStopExecutorService().submit(new
Runnable() {
@Override
public void run() {
try {
- prepareAndStartListenerContainer();
+ prepareAndStartListenerContainer(container);
} catch (Exception e) {
LOG.warn("Error starting listener container on
destination: {}. This exception will be ignored.",
- getDestinationName(), e);
+ getDestinationName(container), e);
}
}
@Override
public String toString() {
- return "AsyncStartListenerTask[" + getDestinationName() +
"]";
+ return "AsyncStartListenerTask[" +
getDestinationName(container) + "]";
}
});
} else {
- prepareAndStartListenerContainer();
+ prepareAndStartListenerContainer(container);
}
}
protected void prepareAndStartListenerContainer() {
- listenerContainer.afterPropertiesSet();
+ prepareAndStartListenerContainer(listenerContainer);
+ }
+
+ private void
prepareAndStartListenerContainer(AbstractMessageListenerContainer container) {
+ if (listenerContainer != container) {
+ // the consumer was stopped (and maybe started again with a new
listener container) before this
+ // asynchronous start task ran
+ LOG.debug("Not starting listener container {} on destination {} as
the consumer no longer uses it", container,
+ getDestinationName(container));
+ return;
+ }
+
+ container.afterPropertiesSet();
// only start listener if auto start is enabled or we are explicit
invoking start later
if (initialized || getEndpoint().isAutoStartup()) {
// should we pre test connections before starting?
if (getEndpoint().isTestConnectionOnStartup()) {
- testConnectionOnStartup();
+ testConnectionOnStartup(container);
}
- startListenerContainer();
+ startListenerContainer(container);
}
- // mark as initialized after the auto-startup check
- initialized = true;
+ containerLock.lock();
+ try {
+ if (listenerContainer == container) {
+ // mark as initialized after the auto-startup check
+ initialized = true;
+ return;
+ }
+ } finally {
+ containerLock.unlock();
+ }
+ // the consumer was stopped while this task was starting the
container, and its stop may have run before
+ // container.start(): stop and destroy it here
+ LOG.debug("Stopping listener container {} on destination {} as the
consumer was stopped while starting it",
+ container, getDestinationName(container));
+ container.stop();
+ container.destroy();
Review Comment:
**Resource leak**: if `container.stop()` throws, `container.destroy()` is
not called, leaving the container in a stopped-but-not-destroyed state — JMS
connections, sessions, and any internal Spring thread pool it holds will leak.
Contrast with `stopAndDestroyListenerContainer(DetachedListenerContainer)`
which correctly wraps both calls in `try/finally`.
```suggestion
try {
container.stop();
} finally {
container.destroy();
}
```
--
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]
