-
Notifications
You must be signed in to change notification settings - Fork 5
Fix WebSocket race condition by adding readyState check before send() #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix WebSocket race condition by adding readyState check before send() #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good to me. I didn't test it though.
|
We have good evidence to expect that this solves the document not rendered issue sometimes happening in Collectives, when notify-push is in use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concurrent call of setupSocket is covered by
if (window._notify_push_ws) {
return true;
}
window._notify_push_ws = true;So it is safe to run setupSocket twice. Or is executed only once.
However, onsent event listener is async (a new macro task), so there is (a tiny) chance that
window._notify_push_ws.readyState === WebSocket.OPEN already, but window._notify_push_ws.onopen has not been executed yet.
Though this is called later anyway in onopen, authentication might have not been happened yet on this send('listen ' + name) call inside listen, and then be called again.
I cannot say if this is a problem, need to try or check the notify_push server source.
For example, I can imagine how response to listen results in Invalid credantials and breaks JSON.parse in the onmessage handler. Or adds complexity to the auth process.
Proposal: instead of relying on window._notify_push_ws.readyState (which does not indicate the setup ready state), add a new flag that actually indicates notify_push client ready state (the setup being complete).
Then we don't need to rely on listen being safe to be called before auth or twice.
|
@ShGKme thank you for the review and for the feedback. In the recent commit, I added the I'm setting the ready flag after the preauth is sent, and before the existing listeners loop runs. It could potentially cause duplicate sends, if a What would be the preferred approach for handling this timing? |
Signed-off-by: Benjamin Frueh <[email protected]>
… Webservice readyState Signed-off-by: Benjamin Frueh <[email protected]>
fabdefd to
fa363d2
Compare
Fix WebSocket race condition causing "Still in CONNECTING state" errors
Problem
When multiple
listen()calls occur while the WebSocket is still connecting, a race condition occurs:listen()call initializes WebSocket (state: CONNECTING)listen()call sees existing WebSocket object and immediately sends without checking connection stateInvalidStateError: Failed to execute 'send' on 'WebSocket': Still in CONNECTING stateThis affects apps like Nextcloud Collectives where the page fails to render due to these WebSocket errors.
Solution
Add readyState check to only send when WebSocket is OPEN. When the WebSocket state is CONNECTING, the
listen()calls are queued inwindow._notify_push_listenersand processed by the onopen handler insetupSocket()once the connection is established.