fix: complete client websocket future on open #20587
Conversation
There was a problem hiding this comment.
Looks good to me.
Tested with the provided reproducible steps and noticed the cases when onOpen happens before whenCompleted, with this fix the connection and message sending seems to run without any deadlocks or errors, see my log below:
2024-12-10 10:56:58,385 DEBUG [com.vaa.bas.dev.vit.ViteWebsocketConnection] (Thread-171) Connected using the vite-hmr protocol
2024-12-10 10:56:58,387 DEBUG [com.vaa.bas.dev.vit.ViteWebsocketProxy] (vert.x-eventloop-thread-0) Sent message to Vite: {"type":"custom","event":"vite-plugin-checker","data":{"event":"runtime-loaded"}}
2024-12-10 10:56:58,387 DEBUG [com.vaa.bas.dev.vit.ViteWebsocketConnection] (Thread-171) Connection to ws://127.0.0.1:51220/VAADIN/ using the vite-hmr protocol established
2024-12-10 10:56:58,387 DEBUG [com.vaa.bas.dev.vit.ViteWebsocketConnection] (HttpClient-24-Worker-0) Message from Vite: {"type":"connected"}
2024-12-10 10:56:58,388 DEBUG [com.vaa.bas.dev.vit.ViteWebsocketProxy] (HttpClient-24-Worker-0) Message sent to browser: {"type":"connected"}
| @@ -72,15 +72,21 @@ public ViteWebsocketConnection(int port, String path, String subProtocol, | |||
| this.onClose = onClose; | |||
| String wsHost = ViteHandler.DEV_SERVER_HOST.replace("http://", "ws://"); | |||
| URI uri = URI.create(wsHost + ":" + port + path); | |||
| clientWebsocket = HttpClient.newHttpClient().newWebSocketBuilder() | |||
| clientWebsocket = new CompletableFuture<>(); | |||
There was a problem hiding this comment.
minor: clientWebsocket could be instantiated in place when declared, IMO it's a little bit clearer for code reading.
|
One more remark, not blocking though, can we replace |
Probably yes, but we should decide what timeouts to apply. |
Completing the websocket future in onOpen event prevents the connection to hang indefintely when application run on low resources. Fixes #20586
mcollovati
force-pushed
the
issues/20586-vitewebsocketconnect-locks
branch
from
8a95b97 to
91d5db9
Compare
|
I wonder if we can proceed without timeouts, and change it if issues will be reported. |
|
Sure, it was just a though for future. |
|
* fix: complete client websocket future on open Completing the websocket future in onOpen event prevents the connection to hang indefintely when application run on low resources. Fixes #20586 * apply review suggestions
* fix: complete client websocket future on open Completing the websocket future in onOpen event prevents the connection to hang indefintely when application run on low resources. Fixes #20586 * apply review suggestions Co-authored-by: Marco Collovati <[email protected]>
Description
Completing the websocket future in onOpen event prevents the connection to hang indefintely when application run on low resources.
Fixes #20586
Type of change
Checklist
Additional for
Featuretype of change