gnodet-bot commented on code in PR #27764:
URL: https://github.com/apache/camel/pull/27764#discussion_r4257612870
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketClientConsumer.java:
##########
@@ -73,10 +75,16 @@ private WebSocket connect(boolean reconnect) throws
Exception {
Lock connectLock = getEndpoint().getClientConsumerConnectLock();
connectLock.lock();
try {
+ // Check before connecting: if the WebSocket is already open, the
setup callback will be invoked on it
+ // (see getWebSocket javadoc), but "opened" must only track
newly-opened sockets so that the reconnect
+ // guard below does not attempt to close the shared endpoint
WebSocket.
+ boolean alreadyOpen = getEndpoint().isWebSocketOpen();
Review Comment:
⚠️ **Unresolved nit (from previous review):** `alreadyOpen` is still sampled
here, outside `getWebSocket`. A producer calling `getWebSocket()` directly does
NOT hold `connectLock`, so the race window exists: the producer can open and
cache a WebSocket between this `isWebSocketOpen()` check (returning `false`)
and the `getWebSocket()` call. When `getWebSocket()` then finds the socket open
and takes the `else` branch, the setup callback runs with `alreadyOpen ==
false`, so `opened.set(ws)` is called. If a reconnect then fires with `mine !=
null && !isStarted()`, it closes the producer's shared socket.
As noted in the original review, this requires a producer send, consumer
stop, and reconnect timer all at the same moment — very unlikely, non-blocking.
But the window is still present. Letting `getWebSocket` communicate whether it
opened the socket (it already knows which branch it took) would eliminate both
the helper method and the race.
--
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]
