Repository navigation
fix(knative): fix goroutine leak in Deploy - #3808
Conversation
knative-prow
Bot
added
size/S
|
Hi @Elvand-Lie. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3808 +/- ##
==========================================
- Coverage 54.00% 53.06% -0.94%
==========================================
Files 200 200
Lines 23687 23694 +7
==========================================
- Hits 12792 12574 -218
- Misses 9660 9898 +238
+ Partials 1235 1222 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gauron99
left a comment
gauron99
left a comment
There was a problem hiding this comment.
Thanks for the PR, I think there is the same issue in the waiter goroutine. Couldn't cherr also be blocked forever? We could make cherr buffered make(chan error, 1) or add the same done check in select as you did above.
677fc36 to
e4b4df8
Compare
|
didn't think about the waiter goroutine blocking on cherr if the main loop exits early. I've updated cherr to be buffered as you suggested and pushed the fix. Thank you @gauron99 |
e4b4df8 to
38096f9
Compare
knative-prow
Bot
added
size/L
38096f9 to
ee1737b
Compare
lkingland
added
ok-to-test
There was a problem hiding this comment.
Pull request overview
Fixes a goroutine leak in the Knative deployer’s new-service Deploy path by ensuring the private-registry polling goroutine can always exit when the main wait logic finishes.
Changes:
- Introduces a sentinel
errPrivateRegistryand refactors the readiness/private-registry wait logic intowaitForReadyOrPrivateRegistry. - Prevents the private-registry polling goroutine from blocking forever by adding a
donesignal and buffering the service-wait error channel. - Adds focused unit tests for
waitForReadyOrPrivateRegistry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/knative/deployer.go | Refactors Deploy wait logic into a helper and adds cancellation signaling to avoid goroutine leaks. |
| pkg/knative/deployer_test.go | Adds unit tests covering success, wait error, private-registry detection, and delayed success scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| waitForService := func() error { | ||
| err, _ := client.WaitForService(ctx, f.Name, | ||
| clientservingv1.WaitConfig{Timeout: k8s.DefaultWaitingTimeout, ErrorWindow: k8s.DefaultErrorWindowTimeout}, | ||
| wait.NoopMessageCallback()) | ||
| cherr <- err | ||
| close(cherr) | ||
| }() | ||
|
|
||
| presumePrivate := false | ||
| main: | ||
| // Wait for either a timeout or a container condition signaling the image is unreachable | ||
| for { | ||
| select { | ||
| case private := <-chprivate: | ||
| if private { | ||
| presumePrivate = true | ||
| break main | ||
| } | ||
| case err = <-cherr: | ||
| break main | ||
| } | ||
| return err | ||
| } | ||
| if presumePrivate { | ||
| err := fmt.Errorf("your function image is unreachable. It is possible that your docker registry is private. If so, make sure you have set up pull secrets https://knative.dev/docs/developer/serving/deploying-from-private-registry") | ||
| return fn.DeploymentResult{}, err | ||
| isPrivateRegistry := func() bool { | ||
| return d.isImageInPrivateRegistry(ctx, client, f) | ||
| } |
ee1737b to
a4df2f7
Compare
knative-prow-robot
added
the
needs-rebase
The Deploy function (new-service creation path) starts a background goroutine that polls isImageInPrivateRegistry every 5 seconds and sends results on an unbuffered chprivate channel. If WaitForService completes first (via cherr), the main function returns and the poller goroutine blocks forever on the channel send with no receiver, leaking the goroutine and captured context. Fixed by adding a done channel that the main select loop closes when it exits. The poller goroutine now selects on done alongside its timer and channel send, so it always exits promptly. Fixes knative#3797
Cancel alone leaves WaitForService and the log follower running until they notice. Deploy returned in that window, so a private-registry exit could leave the waiter alive until its own timeout. Cancel, then wait for each goroutine to finish. Waiting without cancelling first deadlocks: both block until the context ends.
a4df2f7 to
8ffb670
Compare
knative-prow-robot
removed
the
needs-rebase
|
Thanks @Elvand-Lie! Rebased onto main and added a commit on top that cancels and then joins the waiter and log-follower goroutines on every return path (addressing the Copilot note), with tests for the private-registry and caller-cancel exits. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Elvand-Lie, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
approved
Problem
The Deploy function (new-service creation path) starts a background goroutine that polls isImageInPrivateRegistry every 5 seconds and sends results on an unbuffered chprivate channel.
If WaitForService completes first (via cherr), the main function returns and the poller goroutine blocks forever on the channel send with no receiver, leaking the goroutine and captured context.
Fix
Fixed by adding a done channel that the main select loop closes when it exits. The poller goroutine now selects on done alongside its timer and channel send, so it always exits promptly.
Fixes #3797