gnodet-bot commented on code in PR #28049:
URL: https://github.com/apache/camel/pull/28049#discussion_r4306758215
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/MiloClientConnection.java:
##########
@@ -81,14 +98,37 @@ public OpcUaClient getOpcUaClient() {
}
protected void init() {
- this.manager = new SubscriptionManager(this.configuration,
Stack.sharedScheduledExecutor(), 10_000);
+ ObjectHelper.notNull(this.camelContext, "camelContext");
+
+ // A failed request disconnects the client, which can wait for a
reconnect attempt that Milo schedules on its shared
+ // scheduled executor, so this connection handles its requests and
reconnects on an executor of its own.
+ final ThreadPoolProfile profile = new ThreadPoolProfile("MiloClient");
+ // as many threads as that executor; with one processor or with
virtual threads this is one thread, so a route
+ // that continues on it after an awaited request must not wait for
another request of this connection
+ profile.setPoolSize(Runtime.getRuntime().availableProcessors());
+ // not bounded, like that executor, because a rejected reconnect would
end the reconnects of this connection
+ profile.setMaxQueueSize(-1);
+ // a request that completes after the connection is closed fails
instead of being dropped
+ profile.setRejectedPolicy(ThreadPoolRejectedPolicy.Abort);
+ this.executor =
this.camelContext.getExecutorServiceManager().newScheduledThreadPool(this,
"MiloClient", profile);
+
+ this.manager = new SubscriptionManager(this.configuration,
this.executor, 10_000);
}
@Override
public void close() throws Exception {
- if (this.manager != null) {
- this.manager.dispose();
- this.manager = null;
+ lock.lock();
+ try {
+ if (this.manager != null) {
+ this.manager.dispose();
+ this.manager = null;
+ }
+ } finally {
+ if (this.executor != null) {
+
this.camelContext.getExecutorServiceManager().shutdown(this.executor);
+ this.executor = null;
+ }
+ lock.unlock();
Review Comment:
⚠️ **Potential lock leak:** `lock.unlock()` is positioned after
`camelContext.getExecutorServiceManager().shutdown(this.executor)` inside the
`finally` block. If `shutdown()` throws a `RuntimeException`, `lock.unlock()`
would never be called, leaving the lock permanently held.
Consider restructuring to guarantee unlock regardless of executor shutdown
outcome:
```suggestion
@Override
public void close() throws Exception {
lock.lock();
try {
if (this.manager != null) {
this.manager.dispose();
this.manager = null;
}
if (this.executor != null) {
this.camelContext.getExecutorServiceManager().shutdown(this.executor);
this.executor = null;
}
} finally {
lock.unlock();
}
}
```
--
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]
