-
Notifications
You must be signed in to change notification settings - Fork 647
chore: fix goroutine leak #7880
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7880 +/- ##
==========================================
- Coverage 72.83% 72.78% -0.06%
==========================================
Files 235 235
Lines 35176 35177 +1
==========================================
- Hits 25622 25603 -19
- Misses 7741 7757 +16
- Partials 1813 1817 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6799252 to
ce86584
Compare
Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
ce86584 to
09fad20
Compare
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
hey @zhaohuabing the diff looks good, has this been manually tested to make sure there's no indefinite block |
|
/retest |
Yes, I've manually tested this PR in my local kind cluster by changing the envoy-gateway-config configMap. This is also covered by e2e tests - the e2e tests apply the envoy-gateway-config configMap after deploying the EG. |
fix goroutine leak Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com>
fix goroutine leak Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
* fix: set observedGeneration in envoy patch policy (#7715) * fix: set observedGeneration in envoy patch policy Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> * add release note Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> --------- Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> * fix: add validation for request buffer limit (#7687) Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> * fix: setting externalTrafficPolicy for NodePort service type (#7823) Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> * fix: make port-forward worked for OTel collector on port 19001 (#7860) Signed-off-by: zirain <zirain2009@gmail.com> * chore: fix goroutine leak (#7880) fix goroutine leak Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> * fix gen Signed-off-by: zirain <zirain2009@gmail.com> * bump envoy to 1.35.8 Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: Kota Kimura <86363983+kkk777-7@users.noreply.github.com> Co-authored-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Co-authored-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
* fix: set observedGeneration in envoy patch policy (#7715) * fix: set observedGeneration in envoy patch policy Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> * add release note Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> --------- Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * fix: add validation for request buffer limit (#7687) Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * fix: nil pointer error when applying BackendTrafficPolicy to HTTPRoute with no backendRefs (#7765) * fix: checking route section name in backend traffic policy Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * fix: setting externalTrafficPolicy for NodePort service type (#7823) Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * fix: add indexing and processing for CRL references in ClientTrafficPolicies (#7829) Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * feat: change the benchmark report to json format (#6818) * benchmark json output Signed-off-by: zirain <zirain2009@gmail.com> * fix Signed-off-by: zirain <zirain2009@gmail.com> * fix Signed-off-by: zirain <zirain2009@gmail.com> * fix lint Signed-off-by: zirain <zirain2009@gmail.com> * fix Signed-off-by: zirain <zirain2009@gmail.com> * revert Signed-off-by: zirain <zirain2009@gmail.com> * fix seconds Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * bechmark: scale up RPS to test data plane CPU performance (#7810) * Scale up RPS to test data plane CPU performance Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * set duration to 120s Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * discard invalid samples Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * change scrape interval to 10s Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * remove invalid cpu sampling data Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * reduce duration to 60 Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix benchmark end time Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix data plane benchmark start time Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * increase test time to get more samples Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * adjust rps for each scale Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * address comments Signed-off-by: Huabing(Robin) Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing(Robin) Zhao <zhaohuabing@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * fix: make port-forward worked for OTel collector on port 19001 (#7860) Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * chore: fix goroutine leak (#7880) fix goroutine leak Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> * fix gen-check Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> --------- Signed-off-by: kkk777-7 <kota.kimura0725@gmail.com> Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: Huabing(Robin) Zhao <zhaohuabing@gmail.com> Co-authored-by: Kota Kimura <86363983+kkk777-7@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
| runnersDone := make(chan struct{}) | ||
| var hookWG sync.WaitGroup | ||
| hook := func(c context.Context, cfg *config.Server) error { | ||
| hookWG.Add(1) |
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.
This is causing a race condition in the main routine. Waitgroup.Add is not supposed to be called from other goroutine than the waiter. @envoyproxy/gateway-maintainers @nacx unfortunately this is introduced and released already in v1.6.2.
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 have been suggesting since the last year that EG definitely needs to have a -race flag enabled end to end tests. There has been too many race condition like this being fixed and being introduced multiple times. #5539
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.
In other words the so-called "goroutine leak" is not fixed with this PR
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.
IIUC, before this, the chan would be block if the config load more than once.
I would say we fix the goroutinu leak, but introuduce another race.
Hope #7964 will fix it.
fix goroutine leak Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
This PR fixes a potential go routine leak by the
runnerDonechannel - the config loader starts a new go routine for every change and the go routine can block onrunnersDone <- struct{}{}and linger forever. This change ensures those goroutines can exit cleanly.