Repository navigation
[server] init subscriber slices with len=0 in helper constructors - #19643
SarthakB11 wants to merge 1 commit into
Conversation
NewConfigurationHelper and NewSummaryHelper pre-fill subscriber slices
with 10 nil channels via make([]chan struct{}, 10), so iteration in the
matching Publish* methods walks 10 nil entries before any real
subscriber. Sending to a nil channel blocks forever; the pre-existing
utils.IsClosed guard short-circuited (it explicitly returns true on
nil), but it also drained buffered channels of any queued value.
Switch to make([]chan struct{}, 0) so Subscribe* grows the slice with
real channels only. Matches the existing pattern in
NewDashboardK8sResourcesHelper and NewContextHelper.
Fixes meshery#19581.
Signed-off-by: SarthakB11 <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the initialization of channel slices in ConfigurationChannel and SummaryChannel by setting their initial length to 0. The reviewer identified critical data race vulnerabilities in both files, noting that slices are iterated over without synchronization during publication while being modified under a mutex during subscription. Recommendations include protecting all slice accesses with the existing mutexes and utilizing capacity pre-allocation to maintain performance.
| ApplicationsChannel: make([]chan struct{}, 0), | ||
| PatternsChannel: make([]chan struct{}, 0), | ||
| FiltersChannel: make([]chan struct{}, 0), |
There was a problem hiding this comment.
While initializing with a length of 0 correctly prevents iterating over nil slices, the ConfigurationChannel struct implementation contains a significant data race. The Publish* methods (e.g., PublishApplications at line 30) iterate over these slices without any synchronization, while the Subscribe* methods modify them using the mx mutex. Accessing a slice while it is being modified in another goroutine is a data race and can lead to crashes or undefined behavior. You should ensure that all accesses to these slices are protected by the mutex, for example by copying the slice while holding the lock before iterating.
Additionally, to retain the performance benefit of pre-allocation (which the previous code attempted with make(..., 10)), you can use a capacity of 10 with a length of 0.
| ApplicationsChannel: make([]chan struct{}, 0), | |
| PatternsChannel: make([]chan struct{}, 0), | |
| FiltersChannel: make([]chan struct{}, 0), | |
| ApplicationsChannel: make([]chan struct{}, 0, 10), | |
| PatternsChannel: make([]chan struct{}, 0, 10), | |
| FiltersChannel: make([]chan struct{}, 0, 10), |
| func NewSummaryHelper() *SummaryChannel { | ||
| return &SummaryChannel{ | ||
| channel: make([]chan struct{}, 10), | ||
| channel: make([]chan struct{}, 0), |
There was a problem hiding this comment.
Similar to ConfigurationChannel, the SummaryChannel struct suffers from a data race. The Publish method (line 26) reads the channel slice without holding the mx mutex, which is used by the Subscribe method. Iterating over a slice while it is being modified in another goroutine is a data race and can lead to crashes or undefined behavior.
Consider using make([]chan struct{}, 0, 10) to provide an initial capacity while keeping the initial length at 0, maintaining the optimization from the previous implementation.
| channel: make([]chan struct{}, 0), | |
| channel: make([]chan struct{}, 0, 10), |
|
Commit SHA: END-TO-END TESTS
📦 Test Result Summary
⌛ Duration: 1 minutes and 6 seconds Overall Result: 👎 Some tests failed. [Show/Hide] Test Result Details
❌ Failed Test Details The following tests failed. Click on each to see the failure reason: ❌ authenticate as Local provider (Local - local-setup)File Location: Error Message: |
Fixes #19581.
NewConfigurationHelper(server/models/configuration_helper.go:14-20) andNewSummaryHelper(server/models/meshmodel/helper.go:12-16) initialise their subscriber slices withmake([]chan struct{}, 10). The slice has length 10 of zero-value (nil) channels before any subscriber appends.Subscribe*then appends real subscribers onto the slice, so iteration inPublish*walks 10 nil entries first and the real subscribers after.Sending to a nil channel blocks forever. The pre-existing
utils.IsClosedguard short-circuited on nil channels (explicitif ch == nil { return true }), but the same helper also drains buffered channels of any queued value (case <-ch: return true), which is bug #17011's root cause.Change
Switch to
make([]chan struct{}, 0)soSubscribe*grows the slice with real channels only.Matches the existing pattern in
NewDashboardK8sResourcesHelper(server/models/dashboard_k8s_resources.go:14) andNewContextHelper(server/models/k8scontext_helper.go:14), both of which already use length-zero slices.Why a separate PR
The
IsClosedcleanup chain (#19572 → #19580 → #19585) replaces the broken gate. After that chain lands, the publishers no longer block on nil slots (they hit thedefaultbranch of the non-blocking select), but they still silently iterate ten no-op slots per publish. Fixing the root cause at construction is independent and correct on master too.Signed commits