gnodet-bot commented on code in PR #27830:
URL: https://github.com/apache/camel/pull/27830#discussion_r4271768519
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketEndpoint.java:
##########
@@ -197,7 +251,19 @@ protected WebSocket
getWebSocket(java.util.function.Consumer<WebSocket> setupCal
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:
💬 **Nit:** The comment on line 258 says _"a connect that completes from now
on closes its WebSocket"_, but `CompletableFuture.cancel(false)` does not
itself close any WebSocket — it merely marks the future as cancelled so that
any subsequent `complete(ws)` call returns `false`. The actual WebSocket close
(if any) would need to happen inside the Vert.x connect callback when it
detects the cancelled future. The comment conflates the two and could mislead a
reader.
Consider something more precise, e.g.:
```suggestion
// Cancel so that a late-arriving connect callback sees the
future already cancelled and discards its WebSocket
future.cancel(false);
```
--
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]
