Prevent leaking goroutines from websocket connections #253
Conversation
The write to the CloseNotify() channel was blocking, preventing the Read() call from exiting, and preventing ServeHTTP from returning. This meant that requests tended to stay around forever in a deadlocked state. Using the context package prevent this, as the cancellation is non-blocking. It depends on Go 1.7. Note that the writer still needs to support the CloseNotifier interface, otherwise other things break, but it does not seem to be necessary to signal using it.
| @@ -148,14 +149,17 @@ func (w *WrappedGrpcServer) handleWebSocket(wsConn *websocket.Conn, req *http.Re | |||
| return | |||
| } | |||
|
|
|||
| ctx, cancelFunc := context.WithCancel(req.Context()) | |||
| defer cancelFunc() | |||
There was a problem hiding this comment.
Do we need to explicitly cancel this? Can we not rely on the built-in request context being cancelled?
There was a problem hiding this comment.
If we can rely on the request context we don't need to pass around a cancelFunc either. I assume you've tested some of this?
There was a problem hiding this comment.
Yeah, we've got a websocket client that makes a request every second, so the memory leak is really obvious, and I put in some logging for testing (tracking the start and end of handleWebSocket, as well as before and after the cancellation step in the reader).
I tested a bunch of different configurations. Not cancelling the request context also prevented connections from closing properly; I'm not sure why (you would expect w.server.ServeHTTP to pick up from the error returned from the Body that it would need to cancel the request, but this doesn't seem to happen).
I'll admit that I didn't go diving into the lower-level code to see what was going on - this was mostly by trial-and-error.
There was a problem hiding this comment.
OK - I'm not worried that this will cause any real problems, since the request is over once this function has exited, just thought it was a bit overzealous, but if your testing implies the necessity, I'm OK with it!
| @@ -148,14 +149,17 @@ func (w *WrappedGrpcServer) handleWebSocket(wsConn *websocket.Conn, req *http.Re | |||
| return | |||
| } | |||
|
|
|||
| ctx, cancelFunc := context.WithCancel(req.Context()) | |||
| defer cancelFunc() | |||
There was a problem hiding this comment.
OK - I'm not worried that this will cause any real problems, since the request is over once this function has exited, just thought it was a bit overzealous, but if your testing implies the necessity, I'm OK with it!
|
Would like a confirmation from @MarcusLongmuir that we're OK requiring Go 1.7+ for building this now. |
|
Oh, wait, I think I misunderstood. That It's the other cancel call that's necessary - the one in the reader code. |
|
Let me rephrase: can you categorically confirm that just removing the channel doesn't work? I imagine the request context will be cancelled when the client finishes reading. |
|
Yes, I can confirm that. |
|
Alright well then this seems an appropriate fix :). Lets wait for CI to give the go ahead. |
|
2 failures, I've optimistically rerun them because I don't know if they were just flaky browser tests. |
|
Going to give the last error another go 😬 |
|
Just going to give @MarcusLongmuir and @easyCZ a couple of days to put anymore comments on this but LGTM! |
|
OK I think we've had enough time to think about this, going to merge, thanks for your contribution! |
The write to the CloseNotify() channel was blocking, preventing the
Read() call from exiting, and preventing ServeHTTP from returning. This
meant that requests tended to stay around forever in a deadlocked state.
Using the context package prevent this, as the cancellation is
non-blocking. It depends on Go 1.7.
Note that the writer still needs to support the CloseNotifier interface,
otherwise other things break, but it does not seem to be necessary to
signal using it.