gnodet-bot commented on code in PR #27084:
URL: https://github.com/apache/camel/pull/27084#discussion_r4134584053
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -761,9 +761,11 @@ private Connected performConnect() throws Exception {
}
});
- client.connect();
-
try {
+
+ // connect is called inside this try block to ensure that
disconnect is called on exceptions
+ client.connect();
+
// Create subscription synchronously - the create() method blocks
until complete
Review Comment:
**Still outstanding (item 4 from previous review, marked optional):** In
`performAndEvalConnect()` (line ~662), when `this.disposed` is true after
`performConnect()` returns, the method does `return` without calling
`connected.dispose()`. The freshly connected client leaks its TCP connection
and OPC-UA session during shutdown/route stop.
This is the same class of bug as the one this PR fixes — a connected client
not being cleaned up on an early-exit path. A one-liner `connected.dispose();`
before the `return` would close it.
Not blocking merge of the current fix, but worth addressing in the same PR
since it's the same file and pattern.
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -761,9 +761,11 @@ private Connected performConnect() throws Exception {
}
});
- client.connect();
-
try {
+
+ // connect is called inside this try block to ensure that
disconnect is called on exceptions
+ client.connect();
Review Comment:
The fix itself is correct — `client.connect()` is now inside the try block,
so the catch clause will call `client.disconnect()` on failure. 👍
Still outstanding from the previous review: a **regression test** would
strengthen this. For example, connecting with invalid credentials against the
embedded test server and asserting that no sessions/channels accumulate across
reconnect attempts. The `ExplicitCredentialsTest` infra could be reused.
This was item 3 in the previous review — not yet addressed.
--
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]
