Initial websocket support #57

Merged
merged 2 commits into from Jun 24, 2014

Projects

None yet

2 participants

@joekarl
Contributor
joekarl commented Jun 3, 2014

Handles issue #54

Can be tested here http://www.websocket.org/echo.html. Just put in <host>:<PORT>/ws/checks/{check_id}/measurements and hit connect.

Not sure how much logging there should be, but logs websocket connect/disconnect and error states.

Added godep for gorilla/websocket
Added README snippet
Added handling for creating/tearing down broadcast websockets based on checkID.
Logging with remote address

@joekarl joekarl Initial websocket support
Added godep for gorilla/websocket
Added README snippet
Added handling for creating/tearing down broadcast websockets based on checkID.

Websocket connections can now be made at
<host>:<PORT>/ws/checks/{check_id}/measurements

Logging with remote address
908a8c6
@gorsuch
Member
gorsuch commented Jun 4, 2014

๐Ÿ˜

Really nice! I'll do some testing with this tomorrow.

Thanks!

@joekarl
Contributor
joekarl commented Jun 4, 2014

Just realized that the map of websocket clients isn't thread safe so will potentially have issues if canary did run with gomaxprocs > 1.

Easy solution is to lock around access but that kinda defeats the purpose no?

Thoughts?

@joekarl
Contributor
joekarl commented Jun 4, 2014

We'll easier solution is to not run with max procs > 1...

@gorsuch
Member
gorsuch commented Jun 4, 2014

We'll easier solution is to not run with max procs > 1...

Heh, probably not the right solution. As I understand it, at some point in the future, it is intended that the go scheduler automatically scale beyond a single core, making GOMAXPROCS a thing of the past. Regardless, I bet we can make this work.

Can we push the list management into a single goroutine, and have the http handler register / remove connections by passing a message over a channel to the list manger? If possible, that would make things thread safe.

@joekarl
Contributor
joekarl commented Jun 4, 2014

I think a channel backed solution may be overkill here.

This seems to fit the RWMutex use case where we need blanket access to the list of ws clients by a number of go routines most of which just doing reads. (and in all fairness, a channel is implemented with a set of mutexs anyways...). This way we have good read performance 99% of the time (which is the main concern here).

Go wiki - https://code.google.com/p/go-wiki/wiki/MutexOrChannel

Golang blog - (see section on concurrency where they suggest using RWMutex) http://blog.golang.org/go-maps-in-action

RWMutex - http://golang.org/pkg/sync/#RWMutex

Anyways, I'll see what I can throw together to mitigate that problem.

Sent from my iPhone

On Jun 4, 2014, at 7:41 AM, Michael Gorsuch notifications@github.com wrote:

We'll easier solution is to not run with max procs > 1...

Heh, probably not the right solution. As I understand it, at some point in the future, it is intended that the go scheduler automatically scale beyond a single core, making GOMAXPROCS a thing of the past. Regardless, I bet we can make this work.

Can we push the list management into a single goroutine, and have the http handler register / remove connections by passing a message over a channel to the list manger? If possible, that would make things thread safe.

โ€”
Reply to this email directly or view it on GitHub.

@gorsuch
Member
gorsuch commented Jun 4, 2014

๐Ÿ‘

@joekarl joekarl Put websocket handling behind channels
So there can be concurrent access to sending websocket information.
Initially tried a rwMutex but this proved too complicated for minor gain.
8af2aa6
@joekarl
Contributor
joekarl commented Jun 6, 2014

Feeling better about this now. Ended up trying both options, the channel backed one ended up being simpler so went with that.

@joekarl
Contributor
joekarl commented Jun 6, 2014

The way this is setup also leaves open the possibility for two way communication (ie, web socket could potentially ask for gap information completely bypassing http layer). Didn't build that though lol.

@gorsuch
Member
gorsuch commented Jun 8, 2014

This looks awesome @joekarl! I'll be testing this more over the next few days.

@joekarl
Contributor
joekarl commented Jun 8, 2014

Cool, definitely would appreciate another set of eyes on that code.

Sent from my iPhone

On Jun 7, 2014, at 8:32 PM, Michael Gorsuch notifications@github.com wrote:

This looks awesome @joekarl! I'll be testing this more over the next few days.

โ€”
Reply to this email directly or view it on GitHub.

@gorsuch
Member
gorsuch commented Jun 24, 2014

This is great and works exactly as I'd expect it to. Sorry it took me so long to get to this - it's been a busier-than-expected month.

@gorsuch gorsuch merged commit 9ff9f30 into canaryio:master Jun 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment