fix(k8s): thread caller ctx into DialContext exec goroutine#3772
fix(k8s): thread caller ctx into DialContext exec goroutine#3772SarthakB11 wants to merge 1 commit into
Conversation
The exec goroutine inside DialContext was started with context.TODO() even though DialContext receives ctx as a parameter and the outer select watches <-ctx.Done(). When the caller cancels ctx, DialContext returns immediately on the Done case, but the spawned goroutine keeps running because c.exec was given an empty context. The exec stream against kube-apiserver stays open for the lifetime of the parent process. Pass ctx to c.exec so cancellation propagates and the goroutine exits cleanly. closeWithError is idempotent so the existing race-free path through connectFailure / ctx.Done remains intact. Line 323 in Dial is unchanged: the attach goroutine there is intentionally long-lived (held open by emptyBlockingReader on the detach signal) and must outlive Dial. Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SarthakB11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @SarthakB11. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
https://pkg.go.dev/net#Dialer.DialContext
|
matejvasek
left a comment
There was a problem hiding this comment.
You cannot use that context.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3772 +/- ##
=======================================
Coverage 57.03% 57.03%
=======================================
Files 182 182
Lines 21376 21376
=======================================
Hits 12191 12191
Misses 7953 7953
Partials 1232 1232
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Not really as we close the connection. You might replace |
Changes
DialContextreceivesctxas a parameter and the outerselectwatches<-ctx.Done()to return early when the caller cancels. The exec goroutine spawned at line 80, however, calledc.exec(context.TODO(), ...)rather than passingctx. On caller cancellation,DialContextreturns immediately, but the goroutine keeps running and the exec stream against kube-apiserver stays open for the lifetime of the parent process.Pass
ctxtoc.exec. The outer select still racesconnectFailureagainstctx.Done()andconn.closeWithErroris idempotent across both paths.Dialat line 323 is unchanged. Thatattachgoroutine is intentionally long-lived (held open byemptyBlockingReaderwaiting on the detach signal) and must outlive theDialcall./kind bug