Skip to content
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

Prevent trying to open too many connections to the server when the server is closing the websocket on its own #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jggc
Copy link

@jggc jggc commented Jun 14, 2021

See #67

Currently when the server decides to close the websocket after an authentication failure y-websocket immediately retries to connect since the wsUnsuccessfulReconnects is reinitialized as soon as the onopen event occurs.

This pr adds a 1 second delay before resetting the wsUnsuccessfulReconnects which will allow for the reconnection timeout to be greater than 0 and avoid trying to reopen the connection on every tick which can easily overload the server with a few clients.

If the connection is closed within 1 second of opening, the resetting of wsUnsuccessfulReconnects is canceled and the number of wsUnsuccessfulReconnects will keep growing.

setTimeout solution, really?

I'm usually very much against using timers to "fix" a behavior, it's almost always wrong but here the goal is to determine when a connection is successful. A connection that fails to authenticate is obviously not a successful connection and since we can't authenticate using headers with websockets (Except by hacking the protocol header, which I think stinks a little) we need to establish the websocket connection before closing it because of an authentication failure.

The other main reason why I think setTimeout is a proper solution here is that it specifically only affects the reconnection interval logic. It was obviously not intended to try reconnecting hundreds of times a second and this fix still allows for the exact same logic to happen, and to consider a 1 second connection a "successful connection" to reset the reconnection interval algorithm.

Huly®: YJS-727

@jggc jggc changed the title Prevent trying to open too many connections on the server when the server is closing the websocket on its own Prevent trying to open too many connections to the server when the server is closing the websocket on its own Jun 15, 2021
@astahmer
Copy link

Agreed, this seems necessary. Temporary fix on my side is to add a 2s delay server-side before closing the ws if the auth isn't valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants