jcelliott / turnpike

Go implementation of a WAMP (Web Application Messaging Protocol) client and router
MIT License
258 stars 88 forks source link

Fixes for all detectable turnpike concurrency issues #119

Closed djoyner closed 8 years ago

djoyner commented 8 years ago

Fixes #118.

Avoid data races, concurrent map accesses, and concurrent writes to websocket connections.

Instead of sprinkling locks all over the client, realm, broker, and dealer implementations this approach uses a goroutine in each client and realm -- processing closures serially to guarantee exclusion between other goroutines.

Passes unit tests under Golang 1.6 with the race detector turned on.

yanfali commented 8 years ago

Doesn't the sync struct in Publish basically serialize all writes on a topic for publish? Is that strictly necessary?

djoyner commented 8 years ago

@yanfali You are reading it correctly -- this does serialize writes to underlying WS connections. This was strictly necessary throughout the client and server since otherwise gorilla/websocket is prone to panic (see guidelines in https://godoc.org/github.com/gorilla/websocket#hdr-Concurrency). This did not occur in turnpike tests but we've seen it many times in production use.

yanfali commented 8 years ago

I serialized it on the websocket write itself not on the publish. I wonder if this is punishing all subscribers to a topic for something that is a per websocket issue.

On Fri, May 6, 2016, 11:45 David Joyner notifications@github.com wrote:

@yanfali https://github.com/yanfali You are reading it correctly -- this does serialize writes to underlying WS connections. This was strictly necessary throughout the client and server since otherwise gorilla/websocket is prone to panic (see guidelines in https://godoc.org/github.com/gorilla/websocket#hdr-Concurrency). This did not occur in turnpike tests but we've seen it many times in production use.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/jcelliott/turnpike/pull/119#issuecomment-217526541

djoyner commented 8 years ago

@yanfali I do agree that there's opportunity to pursue finer-grained strategies.

djoyner commented 8 years ago

@yanfali Thinking about your comments more, I think it's important to fix now for performance reasons. I'll create another PR to address. Also, I think #117 is helpful.

yanfali commented 8 years ago

:+1: