- Notifications
You must be signed in to change notification settings - Fork928
fix: use a background context when piping derp connections#6750
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
105c88f to139f905Compare| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wrote this on the previous version of this PR, but I guess the suggestion still applies:
How do we avoid leaks here? Isleft guaranteed to be closed at an appropriate time and propagate toright?
Should we actually doctx, cancel := context.WithCancel(api.ctx) outside here and callcancel() inagentConn.CloseFunc?
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.
Ahh, good point! Will fix.
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.
Fixed!
e7a63dd to7798533CompareThere 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.
Approving albeit uncommon use of context.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed, I'm going to remove the ctx here now!
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.
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
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.
Looks good!
This was causing boatloads of connects to reestablish every time...
See#6746