Gracefully close websocket connections #3638
Conversation
john-markham
force-pushed
the
jm/fix-websockets
branch
6 times, most recently
from
744d00d to
e9aed71
Compare
| @@ -283,6 +295,16 @@ func (c *wsConnection) run() { | |||
| go c.closeOnCancel(ctx) | |||
|
|
|||
| for { | |||
| c.mu.Lock() | |||
There was a problem hiding this comment.
This will have performance implications
There was a problem hiding this comment.
Yeah totally. I'm open to suggestions as to how we should measure how much this impacts performance as this is a concern for me and my team as well.
Overall though - if the thesis of this PR is correct (that we are not gracefully handling shutdown), I'm inclined to trade perf off to get us closer to WS spec compliance.
There was a problem hiding this comment.
Ideally we have a solution that does both: doesn't impact perf and gracefully shuts down the connection. Again open to suggestions on how to refactor the implementation to do both.
| @@ -231,6 +238,11 @@ func (c *wsConnection) init() bool { | |||
|
|
|||
| func (c *wsConnection) write(msg *message) { | |||
| c.mu.Lock() | |||
| // don't write anything to a closed / closing connection | |||
| if c.serverClosed { | |||
| c.mu.Unlock() | |||
There was a problem hiding this comment.
I would move that as defer c.mu.Unlock() right after the lock
john-markham
force-pushed
the
jm/fix-websockets
branch
from
e9aed71 to
7920ef4
Compare
Fixes #3633 as follow-up to discussion #2847
This is intended to gracefully close WebSocket connections, because the Gorilla/websocket doesn't provide us a nice way to do that.
I have: