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

Improved connection error handling in the client #67

Closed
hanspagel opened this issue May 20, 2021 · 2 comments · Fixed by ueberdosis/hocuspocus#163
Closed

Improved connection error handling in the client #67

hanspagel opened this issue May 20, 2021 · 2 comments · Fixed by ueberdosis/hocuspocus#163
Assignees
Labels
enhancement New feature or request

Comments

@hanspagel
Copy link
Contributor

hanspagel commented May 20, 2021

Description
We’re building an open source WebSocket server based on y-websocket, called hocuspocus. A lot of developers struggle to debug issues with it, for example when authentication fails. We’d like to improve the experience for developers.

For example, currently, the y-websocket client tries to reconnect, no matter what the error is, without communicating a reason. That can be frustrating to debug.

Also, that could cause a DDOS attack on the server in a production environment, even with a few users, when they all try to reconnect again and again. (We had this in the tiptap documentation example.)

The solution I’d like to have
I’d like to implement an error handling in the client, but I’m not sure if that’s in the scope of this project and we should send a PR or create a fork of y-websocket.

For the “Authentication failed“ example the flow could look like that:

  • The authentication fails (e. g. unknown token)
  • The backend sends a WebSocket message, and/or use WebSocket CloseEvent codes to communicate the error to the client.
  • The client stops trying to reconnect, or slows down the reconnection attempts.
  • The error could be logged to the console, making debugging easier.
  • (Optional) Enabling/disabling console output could be done based on an environment variable and/or based on a setting

Alternatives I’ve considered

  1. Creating a PR, but it’s probably hard to keep it consistent in y-websocket client, server and hocuspocus?
  2. Creating a fork, would allow us to add enhancements freely, but doubles the burden of maintenance.
  3. Not doing anything, but that’s causing a lot of work with feedback/issues/emails with people needing help to debug.

Honestly, not sure what’s the best way. I’d love to have your opinion on this.

Additional context
There’s already code in the client to slow down reconnects, but that doesn’t seem to work in our environments or at least in some cases, but I couldn’t find time to debug that. Fixing/improving that would at least help with “DDOS attacks” while developing, but probably not that much with debugging. Could be a first step though.

Using the WebSocket CloseEvent with y-websocket: #7 (comment)
Stale PR, adding the WebSocket CloseEvent to the status event: https://github.com/yjs/y-websocket/pull/59/files#diff-bf6cbbe721bbeff64c972b548f6291dc24bba8fd0e1cef5b91d87513c6a0b362R105
Stale PR, maybe I can finish that one? #48

@jggc
Copy link

jggc commented Jun 14, 2021

I ran into exactly this today and made a fix similar to the open prs but way dirtier, I throw directly from the onclose event before the connection retry is attempted when the error code is my authentication error code (4401)

Maybe a very simple fix would be to execute a custom onclose handler within the current handler and this custom handler could act like an express middleware and determine if it wants to run next() which would be the reconnect logic.

Another option : fix the DDOS isue which I think removes a lot of the emergency of fixing the current issue.

We could simply track the time between the onopen and the onclose event and if it's been less than ~1 second consider this as a connection failure. I'll submit a pr for this.

@jamesopti
Copy link

I just hit this recently when my Hocuspocus server hit an error in its onConnect hook, effectively causing a DOS to the server as the client tried to reconnect indefinitely.

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

Successfully merging a pull request may close this issue.

4 participants