fix goroutine leak in watchMux #4221
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
karmada-bot
added
the
size/M
ikaven1024
force-pushed
the
fix/watchMux-leak
branch
2 times, most recently
from
496da7d
to
44a24ee
Compare
karmada-bot
added
size/L
|
cc @xigang |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4221 +/- ##
==========================================
- Coverage 52.78% 52.77% -0.01%
==========================================
Files 239 239
Lines 23584 23585 +1
==========================================
Hits 12448 12448
Misses 10460 10460
- Partials 676 677 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
ikaven1024
force-pushed
the
fix/watchMux-leak
branch
from
44a24ee
to
d193e81
Compare
pkg/search/proxy/store/util.go
Outdated
| if chosen == doneCaseIndex { | ||
| // Received from done chan. | ||
| // In fact, this will never happen | ||
| panic(fmt.Sprintf("unexpectedly receive from done chan: %v", val.Interface())) |
There was a problem hiding this comment.
If panic is triggered, does the exception need to be caught?
pkg/search/proxy/store/util.go
Outdated
| }() | ||
| defer close(w.result) | ||
| for { | ||
| chosen, val, ok := reflect.Select(cases) |
There was a problem hiding this comment.
The performance of reflect.Select is worse than that if select. if the scale of the federate cluster is too large, there may be performance issue. short-tem that's no problem.
There was a problem hiding this comment.
dynamic select case cost twice time of static select. I test it with 1000 source, 10000 events:
goos: darwin
goarch: arm64
BenchmarkSelectCaseStatic
BenchmarkSelectCaseStatic-8 2 740559750 ns/op
BenchmarkSelectCaseDynamic
BenchmarkSelectCaseDynamic-8 1 1627772333 ns/op
fix: fix exec failure with karamada-aggregated-apiserver Signed-off-by: yingjinhui <yingjinhui@didiglobal.com>
ikaven1024
force-pushed
the
fix/watchMux-leak
branch
from
d193e81
to
3c07a34
Compare
karmada-bot
added
size/S
| @@ -246,19 +257,8 @@ func (w *watchMux) startWatchSource(source watch.Interface, decorator func(watch | |||
| select { | |||
| case <-w.done: | |||
| return | |||
| default: | |||
| case w.result <- copyEvent: | |||
There was a problem hiding this comment.
@ikaven1024 @XiShanYongYe-Chang Is there a data race when multiple goroutines write w.result channel?
There was a problem hiding this comment.
Writing a channel in multiple goroutines is safe, there is no data race.
| } | ||
|
|
||
| go func() { | ||
| // close result chan after all goroutines exit, avoiding data race. |
There was a problem hiding this comment.
Just a question, why would there be data race in the previous approach?
There was a problem hiding this comment.
Data race happens when one goroutine is wirting result chan, while another goroutine is closing result chan in Stop function.
There was a problem hiding this comment.
@ikaven1024 explained it at #4212 (comment).
|
Migrate to #4212 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4212
Special notes for your reviewer:
Does this PR introduce a user-facing change?: