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

Serialize write access to websocket #115

Closed
wants to merge 2 commits into from
Closed

Conversation

yanfali
Copy link

@yanfali yanfali commented Mar 15, 2016

As the gorilla websocket documentation makes clear, it is the callers
responsibility to ensure there is only 1 concurrent reader and writer to
every websocket.

While investigating alternative Broker implementations which are
asynchronous, I was able to trigger go data race warnings while writing
to the socket from subscriptions which were writing from different
goroutines to the same client.

Add a simple mutex on the websocketPeer to gate writers on Send.
Looking at the current implementation there should only be one reader
from the websocket a time which writes to a messages channel.

This should help with issue #105 and is low hanging fruit. It is the most obvious fix until the code is refactored to use a Send message channel where all the writes would be serialized by the channel.

@yanfali
Copy link
Author

yanfali commented Mar 15, 2016

I see almost the same patch is in Pull request #108

As the gorilla websocket documentation makes clear, it is the callers
responsibility to ensure there is only 1 concurrent reader and writer to
every websocket.

While investigating alternative Broker implementations which are
asynchronous, I was able to trigger go data race warnings while writing
to the socket from subscriptions which were writing from different
goroutines to the same client.

Add a simple mutex on the websocketPeer to gate writers on Send.
Looking at the current implementation there should only be one reader
from the websocket a time which writes to a messages channel.
@yanfali yanfali closed this Mar 16, 2016
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.

1 participant