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

Do not kill entire chat window when websocket closes. #292

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ProjectMoon
Copy link

Reopens the websocket on close. Prevents badly configured reverse proxies from making the application stop working. In my testing, I've noticed that the websocket simply reopens when the connection is closed. Instead of setting error state to true on websocket close, we just clear the websocket out using the provided React setWs function.

Reopens the websocket on close. Prevents badly configured reverse proxies from making the application stop working.
@ItzCrazyKns
Copy link
Owner

In case of a valid reason from the backend, if the WS is closed then it would constantly try to reconnect and then the error message would pop up again and again so I cannot merge this.

@ProjectMoon
Copy link
Author

ProjectMoon commented Jul 30, 2024

Is there a way to communicate the reason then? There needs to be a better solution than just putting the entire page into error state.

Maybe exponential backoff? Attempt to reconnect immediately, then if it fails, back off for X seconds, then more seconds, and so on.

@ProjectMoon
Copy link
Author

Alternatively, can check the error state when onClose. That will make it be able to prevent reconnect attempts if not just a normal close.

I wouldn't just close the entire PR, at least.

@ItzCrazyKns
Copy link
Owner

Alternatively, can check the error state when onClose. That will make it be able to prevent reconnect attempts if not just a normal close.

I wouldn't just close the entire PR, at least.

There are certain states when no error is being sent and the WS close

@ItzCrazyKns ItzCrazyKns reopened this Jul 30, 2024
@ProjectMoon
Copy link
Author

In that case, wouldn't we want to try immediate reconnection then? And exponential back off would handle that issue if we cannot reconnect.

@ProjectMoon
Copy link
Author

Also, after testing this more locally by killing the backend server and thus the websocket connection, it doesn't appear to attempt to reconnect. But this test only covers a sudden connection drop from the backend.

@ProjectMoon
Copy link
Author

Small update: Do not set connection to null if in error state. I'm still trying to work out the best way to do exponential backoff in the current architecture. Do you have any suggestions?

@ItzCrazyKns
Copy link
Owner

ItzCrazyKns commented Jul 31, 2024

State does not gets updated instantly (they are asnyc without a callback), if the WS closes and the error state takes time to be set, the page would still be set to null and this would repeat

@ProjectMoon
Copy link
Author

How about something like awaiting the setError, and then using setTimeout with a 0 interval to wait until the next event loop tick for checking the value?

@ItzCrazyKns
Copy link
Owner

How about something like awaiting the setError, and then using setTimeout with a 0 interval to wait until the next event loop tick for checking the value?

Wait won't work as it doesn't returns a callback function, the setTimeout method is not efficient, 0 ms interval it would check the value a lot of time and might suck the resources.

@ProjectMoon
Copy link
Author

When using setTimeout with a delay of 0, it forces an execution on next event loop. It's kinda like process.nextTick in node. In any case, what is your suggested way?

@ProjectMoon
Copy link
Author

Latest iteration has useRef for reconnecting websocket with exponential backoff. And as a side-note, I learned that useRef existed today.

@ProjectMoon ProjectMoon force-pushed the master branch 2 times, most recently from 963be36 to ad1f3ee Compare August 1, 2024 15:29
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.

3 participants