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

Refactor websocket close logic #102

Merged
merged 2 commits into from
Jul 17, 2021
Merged

Conversation

lorenzodonini
Copy link
Owner

  • Fixes potential deadlocks and panics, that may occur when closing websockets
  • Streamlines websocket close logic for client and server
  • Gracefully closes all clients, after calling Stop on the websocket server
  • Fixed typos in documentation

Signed-off-by: Lorenzo Donini <lorenzo.donini90@gmail.com>
- improved logic when stopping the server to gracefully shutdown every connection
- add ServerStopAllConnections test

Signed-off-by: Lorenzo Donini <lorenzo.donini90@gmail.com>
@michaelbeaumont
Copy link
Contributor

There's no reason to have different readPump/writePump methods for client and server is there? Or put another way, I think the right abstraction might be "connection" and "server", where a server just manages multiple connections.

@lorenzodonini
Copy link
Owner Author

lorenzodonini commented Jul 14, 2021

There's no reason to have different readPump/writePump methods for client and server is there? Or put another way, I think the right abstraction might be "connection" and "server", where a server just manages multiple connections.

There are a few subtle differences between the two, like:

  • the ping/pong mechanism (which adds an additional timer channel and two code blocks, different on each entity)
  • the auto-reconnect mechanism, which is only triggered in the client
  • message handling callback
  • closing logic and error handling

I could definitely abstract this away with a few function parameters, pass a nil channel to the entity that doesn't send the ping and add a couple more conditions, but I'm afraid it won't read as nicely and look as clean.
I'm willing to give it a try, if you believe it would be worth it, but in general I'm not against code duplication, when two entities actually behave differently.

@lorenzodonini lorenzodonini merged commit c6b416e into master Jul 17, 2021
@lorenzodonini lorenzodonini deleted the refactoring/websocket-close branch July 17, 2021 17:53
@lorenzodonini
Copy link
Owner Author

I merged this one for now, will tackle your suggestion in a separate MR.

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