yjs / y-websocket

Websocket Connector for Yjs
https://docs.yjs.dev/ecosystem/connection-provider/y-websocket
MIT License
499 stars 258 forks source link

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

Open jggc opened 3 years ago

jggc commented 3 years ago

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

astahmer commented 3 years ago

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.