- Notifications
You must be signed in to change notification settings - Fork915
fix: fix goroutine leak in log streaming over websocket#15709
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e379c84 to2ae0a74CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Small suggestions but otherwise LGTM 👍🏻
coderd/workspaceagents.go Outdated
| ctx, wsNetConn:= codersdk.WebsocketNetConn(ctx,conn, websocket.MessageText) | ||
| deferwsNetConn.Close() // Also closes conn. | ||
| encoder:=wsjson.NewEncoder[[]codersdk.WorkspaceAgentLog](conn, websocket.MessageText) | ||
| deferencoder.Close(websocket.StatusGoingAway) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we use going away instead of normal closure? It's kind of an error state and a divergence from before (i.e. status from callingwsNetConn.Close()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I chose GoingAway because one likely reason for the server closing the connection is that it's shutting down. We could also be closing because there is a new build and this agent is no longer current.
If we actually used status codes for anything we'd want to send different codes in these cases, but we don't. I can change it to match the oldwsNetConn for consistency.
| @@ -767,7 +758,7 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) { | |||
| err := ws.Ping(ctx) | |||
| cancel() | |||
| if err != nil { | |||
| _ =nconn.Close() | |||
| _ =ws.Close(websocket.StatusGoingAway, "ping failed") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍🏻
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
codersdk/wsjson/decoder.go Outdated
| // nolint: revive // complains that Encoder has the same function name | ||
| func (d *Decoder[T]) Close() error { | ||
| err := d.conn.Close(websocket.StatusGoingAway, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as previously, why not use normal closure? Also, why not take status like encoder for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've switched to StatusNormalClosure.
Here I don't want to take the websocket status like Encoder because it's useful for the Decoder to implementio.Closer to more easily fit into existing code.
2ae0a74 to76911bbCompare148a5a3 intomainUh oh!
There was an error while loading.Please reload this page.
Merge activity
|
fixes#14881Our handlers for streaming logs don't read from the websocket. We don't allow the client to send us any data, but the websocket library we use requires reading from the websocket to properly handle pings and closing. Not doing so can [can cause the websocket to hang on write](coder/websocket#405), leaking go routines which were noticed in#14881.This fixes the issue, and in process refactors our log streaming to a encoder/decoder package which provides generic types for sending JSON over websocket.I'd also like for us to upgrade to the latesthttps://github.com/coder/websocket but we should also upgrade our tailscale fork before doing so to avoid including two copies of the websocket library.(cherry picked from commit148a5a3)
fixes#14881Our handlers for streaming logs don't read from the websocket. We don't allow the client to send us any data, but the websocket library we use requires reading from the websocket to properly handle pings and closing. Not doing so can [can cause the websocket to hang on write](coder/websocket#405), leaking go routines which were noticed in#14881.This fixes the issue, and in process refactors our log streaming to a encoder/decoder package which provides generic types for sending JSON over websocket.I'd also like for us to upgrade to the latesthttps://github.com/coder/websocket but we should also upgrade our tailscale fork before doing so to avoid including two copies of the websocket library.(cherry picked from commit148a5a3)
Uh oh!
There was an error while loading.Please reload this page.
fixes#14881
Our handlers for streaming logs don't read from the websocket. We don't allow the client to send us any data, but the websocket library we use requires reading from the websocket to properly handle pings and closing. Not doing so cancan cause the websocket to hang on write, leaking go routines which were noticed in#14881.
This fixes the issue, and in process refactors our log streaming to a encoder/decoder package which provides generic types for sending JSON over websocket.
I'd also like for us to upgrade to the latesthttps://github.com/coder/websocket but we should also upgrade our tailscale fork before doing so to avoid including two copies of the websocket library.