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

Major refactory with re-connection support #13

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

Conversation

tinchoz49
Copy link

Hi @soyuka ! how are you?

First of all, thanks for you work on this module it was really necessary and we have been using it for the last months.

We start having different issues of re-connection problems + some crash trying to send new messages on closed sockets. We try to fix it but at the end we did a refactor of the code and we add sockette for the reconnection problem.

I don't know if our work can help you but we just want to share it with you in a PR so you can check it.

@soyuka
Copy link
Owner

soyuka commented Mar 15, 2019

Hello ! Thanks for sharing your patch it's really appreciated!

We start having different issues of re-connection problems + some crash trying to send new messages on closed sockets.

This is an interesting issue, I'd love to find a way to reproduce and unit test this against the signalhubws library. Did you found a way to reproduce this programmatically ?

We try to fix it but at the end we did a refactor of the code and we add sockette for the reconnection problem.

I'm not sure how using a new module fixes the issue and I wish we could've fixed this without this new dependency. Also you changed websocket for ws? What polyfil does it use when running on nodejs?

@RangerMauve @G-Ray do you guys have an opinion about this? Did you encounter the same issue?

@RangerMauve
Copy link
Collaborator

Regarding ws vs websocket, I usually use the websocket-stream module which uses ws under the hood so I'm +1 for that.

Sockette is pretty small so if it brings reconnection logic, that could be useful. It's unit tests kind of suck though. 😅

@tinchoz49
Copy link
Author

tinchoz49 commented Mar 15, 2019

Also you changed websocket for ws? What polyfil does it use when running on nodejs?

Something that I don't like from our version is that we have to remove the possibility of setting by the user the WebSocketClass because sockette only use the global one, so if you are in nodejs we set the ws module into the global.WebSocket but that is something that we can change it if just take the reconnection idea of sockette.

This is an interesting issue, I'd love to find a way to reproduce and unit test this against the signalhubws library. Did you found a way to reproduce this programmatically ?

Yes, I think having tests about this is necessary but the errors happens some times 🤦‍♂️ so it's complicated haha.

Prev condition: Browser with a ws socket connection established.

  • Case One: Suspend the machine or put offline your mobile (if you are working from there) and wait a couple of minutes and try to back again. The re-connection never happened, we had to refresh the browser.

  • Case Two: For x reason the ws server shutdown and we lost the connection but the socket client keep sending messages and crashing. When the server is back, the socket client cannot recover from that point.

@soyuka
Copy link
Owner

soyuka commented Mar 18, 2019

Something that I don't like from our version is that we have to remove the possibility of setting by the user the WebSocketClass because sockette only use the global one.

This is blocking to me.

Case One: Suspend the machine or put offline your mobile (if you are working from there) and wait a couple of minutes and try to back again. The re-connection never happened, we had to refresh the browser.

This should be testable. I'm going to try to work on testing it if I find the time.

Case Two: For x reason the ws server shutdown and we lost the connection but the socket client keep sending messages and crashing. When the server is back, the socket client cannot recover from that point.

We should try to use event sourcing for this it's way more appropriated then using websocket haha. Anyway it's a debate for another day.

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