fix: use a background context when piping derp connections #6750
Conversation
105c88f to
139f905
Compare
| @@ -437,7 +435,7 @@ func (api *API) dialWorkspaceAgentTailnet(r *http.Request, agentID uuid.UUID) (* | |||
| defer left.Close() | |||
| defer right.Close() | |||
| brw := bufio.NewReadWriter(bufio.NewReader(right), bufio.NewWriter(right)) | |||
| api.DERPServer.Accept(ctx, right, brw, r.RemoteAddr) | |||
| api.DERPServer.Accept(ctx, right, brw, "internal") | |||
There was a problem hiding this comment.
I wrote this on the previous version of this PR, but I guess the suggestion still applies:
How do we avoid leaks here? Is left guaranteed to be closed at an appropriate time and propagate to right?
Should we actually do ctx, cancel := context.WithCancel(api.ctx) outside here and call cancel() in agentConn.CloseFunc?
There was a problem hiding this comment.
Ahh, good point! Will fix.
e7a63dd to
7798533
Compare
coderd/workspaceagents.go
Outdated
| @@ -414,10 +414,8 @@ func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Req | |||
| httpapi.Write(ctx, rw, http.StatusOK, portsResponse) | |||
| } | |||
|
|
|||
| func (api *API) dialWorkspaceAgentTailnet(r *http.Request, agentID uuid.UUID) (*codersdk.WorkspaceAgentConn, error) { | |||
| ctx := r.Context() | |||
| func (api *API) dialWorkspaceAgentTailnet(ctx context.Context, agentID uuid.UUID) (*codersdk.WorkspaceAgentConn, error) { | |||
There was a problem hiding this comment.
Passing a context to a function that returns something tied to that context is a bit weird and uncommon (think net.Dial, would be awkward if it closed the connection once the context is cancelled).
Not a huge issue, but could trip someone up.
There was a problem hiding this comment.
Agreed, I'm going to remove the ctx here now!
There was a problem hiding this comment.
Removed. Give it one last look please just to confirm I didn't do anything wacky!
This was causing boatloads of connects to reestablish every time... See #6746
This was causing boatloads of connects to reestablish every time...
See #6746