gnodet-bot commented on code in PR #27904:
URL: https://github.com/apache/camel/pull/27904#discussion_r4284738595
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketEndpoint.java:
##########
@@ -182,22 +236,41 @@ protected WebSocket
getWebSocket(java.util.function.Consumer<WebSocket> setupCal
client.connect(connectOptions).onComplete(result -> {
if (result.succeeded()) {
WebSocket ws = result.result();
+ if (future.isDone()) {
+ // the caller gave up waiting (see below): nobody
would use or close this WebSocket
+ ws.close();
+ return;
+ }
LOG.info("Connected to WebSocket on {}",
ws.remoteAddress());
if (setupCallback != null) {
try {
- setupCallback.accept(ws);
+ setupCallback.accept(ws, false);
} catch (Exception e) {
+ ws.close();
future.completeExceptionally(e);
return;
}
}
- future.complete(ws);
+ if (!future.complete(ws)) {
+ ws.close();
+ }
} else {
- webSocket = null;
future.completeExceptionally(result.cause());
}
});
- webSocket = future.get(resolvedClientOptions.getConnectTimeout(),
TimeUnit.MILLISECONDS);
+ try {
+ webSocket =
future.get(resolvedClientOptions.getConnectTimeout(), TimeUnit.MILLISECONDS);
+ } catch (Exception e) {
+ webSocket = null;
+ // a connect that completes from now on closes its WebSocket
+ future.cancel(false);
Review Comment:
⚠️ **Race window: WebSocket can leak between `get()` timeout and
`cancel(false)`**
`CompletableFuture.get(timeout)` does **not** transition the future to a
terminal state when it times out — it merely throws `TimeoutException` and
returns. Between that throw (line 262) and the `future.cancel(false)` call
(this line), the Vert.x callback can run on another thread:
```
Caller thread Vert.x thread
────────────────────────────────────────────────────────
get(timeout) → TimeoutException
ws = result.result()
future.isDone() → false (still
RUNNING)
setupCallback.accept(ws, false) //
handlers registered
future.complete(ws) → true //
future now DONE
webSocket = null
future.cancel(false) → false // cancel fails — future already done
throw e
// ws is never closed; nobody holds a reference to it → LEAK
```
**Fix:** after `future.cancel(false)`, check whether the future already
completed (meaning `cancel` returned `false`) and explicitly close the leaked
WebSocket:
```java
} catch (Exception e) {
webSocket = null;
future.cancel(false);
// If cancel() failed, the callback completed the future between our
timeout and this cancel().
// The WebSocket it stored is orphaned — close it.
if (future.isDone() && !future.isCompletedExceptionally() &&
!future.isCancelled()) {
try { future.getNow(null).close(); } catch (Exception ignored) {}
}
throw e;
}
```
This race is narrow but real — the fix in main (PR #27764) has the same gap.
Worth fixing in both places.
--
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]
