[server] Replace remaining broken utils.IsClosed gates with non-blocking sends by SarthakB11 · Pull Request #19580 · meshery/meshery

SarthakB11 · 2026-05-21T09:58:04Z

The IsClosed[K] helper at server/helpers/utils/utils.go:151 does case <-ch: return true on the passed channel. For a buffered channel that is open but has a queued value, the receive branch fires and the value is dropped. The Publish* helpers that use it as a gate then report the channel closed and skip the send, so events are silently lost.

This is the follow-up I flagged in #19572 for the remaining five utils.IsClosed callers outside event_broadcast.go.

Changes

server/models/configuration_helper.go: PublishApplications, PublishPatterns, PublishFilters
server/models/dashboard_k8s_resources.go: PublishDashboardK8sResources
server/models/meshmodel/helper.go: SummaryChannel.Publish
server/models/k8scontext_helper.go: PublishContext (the inner select was already non-blocking; only the redundant IsClosed gate is removed)

Each gate is replaced with:

for _, ch := range listeners {
    select {
    case ch <- struct{}{}:
    default:
    }
}

utils.IsClosed itself is left in place because event_broadcast.go still references it on master. Removal will be a follow-up once #19572 merges and no callers remain.

Why non-blocking

None of these four helper types expose an unsubscribe path that calls close(ch). Subscribers register a channel and are responsible for their own channel lifecycle, so a send on a closed channel would still panic; that case is not introduced by this PR and is not supported by these helpers either before or after. The non-blocking select preserves the previous "best-effort delivery" intent, handles slow subscribers without blocking the publisher or other subscribers, and avoids the buffered-channel drain bug entirely.

Signed commits

Yes, I signed my commits.

…ing sends The IsClosed[K] helper at server/helpers/utils/utils.go:151 does 'case <-ch: return true' on the passed channel. For a buffered channel that is open but has a queued value, the receive branch fires and the value is dropped. The 'Publish*' helpers that use it as a gate then report the channel closed and skip the send, so events are silently lost. Replaced the gate with a non-blocking 'select { case ch <- struct{}{}: default: }' in: - server/models/configuration_helper.go: PublishApplications, PublishPatterns, PublishFilters - server/models/dashboard_k8s_resources.go: PublishDashboardK8sResources - server/models/meshmodel/helper.go: SummaryChannel.Publish - server/models/k8scontext_helper.go: PublishContext (the inner select was already non-blocking; only the redundant IsClosed gate is removed) Same fix pattern as PR meshery#19572 for server/models/event_broadcast.go. None of these helpers expose an unsubscribe path that closes the channel, so the send-on-closed-channel risk is bounded to callers that close their own channels directly, and the non-blocking select handles the slow-subscriber case identically to the broadcaster. IsClosed itself is left in place because event_broadcast.go still references it on master. Removal will be a follow-up once meshery#19572 merges and no callers remain. Signed-off-by: SarthakB11 <[email protected]>

welcome · 2026-05-21T09:58:07Z

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while performing a commit.

gemini-code-assist

Code Review

This pull request replaces unsafe channel closure checks with non-blocking select statements across several models, including ConfigurationChannel and SummaryChannel. While this improves the robustness of channel operations, the reviewer identified multiple instances where iterating over subscriber slices without holding a mutex leads to potential data races. It is recommended to implement a copy-under-lock pattern in the Publish methods to ensure thread safety during concurrent subscriptions.

gemini-code-assist · 2026-05-21T10:00:38Z

 	for _, ch := range c.ApplicationsChannel {
-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+		select {
+		case ch <- struct{}{}:
+		default:
 		}
 	}


The iteration over c.ApplicationsChannel is performed without holding the mutex c.mx. Since SubscribeApplications modifies this slice under a lock, this constitutes a data race. To fix this, you should copy the slice while holding the lock before iterating, following the pattern already used in PublishContext in server/models/k8scontext_helper.go.

Suggested change

for _, ch := range c.ApplicationsChannel {

if !utils.IsClosed(ch) {

ch <- struct{}{}

select {

case ch <- struct{}{}:

default:

}

}

c.mx.Lock()

subscribers := make([]chan struct{}, len(c.ApplicationsChannel))

copy(subscribers, c.ApplicationsChannel)

c.mx.Unlock()

for _, ch := range subscribers {

select {

case ch <- struct{}{}:

default:

}

}

All 5 Publish sites now copy under lock in 181f7da. Pattern matches PublishContext.

gemini-code-assist · 2026-05-21T10:00:38Z

 	for _, ch := range c.PatternsChannel {
-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+		select {
+		case ch <- struct{}{}:
+		default:
 		}
 	}


Similar to PublishApplications, this function accesses c.PatternsChannel without synchronization, leading to a potential data race with SubscribePatterns.

Suggested change

for _, ch := range c.PatternsChannel {

if !utils.IsClosed(ch) {

ch <- struct{}{}

select {

case ch <- struct{}{}:

default:

}

}

c.mx.Lock()

subscribers := make([]chan struct{}, len(c.PatternsChannel))

copy(subscribers, c.PatternsChannel)

c.mx.Unlock()

for _, ch := range subscribers {

select {

case ch <- struct{}{}:

default:

}

}

gemini-code-assist · 2026-05-21T10:00:38Z

 	for _, ch := range c.FiltersChannel {
-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+		select {
+		case ch <- struct{}{}:
+		default:
 		}
 	}


Similar to PublishApplications, this function accesses c.FiltersChannel without synchronization, leading to a potential data race with SubscribeFilters.

c.mx.Lock() subscribers := make([]chan struct{}, len(c.FiltersChannel)) copy(subscribers, c.FiltersChannel) c.mx.Unlock() for _, ch := range subscribers { select { case ch <- struct{}{}: default: } }

gemini-code-assist · 2026-05-21T10:00:38Z

 	for _, ch := range d.ResourcesChan {
-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+		select {
+		case ch <- struct{}{}:
+		default:
 		}
 	}


The iteration over d.ResourcesChan is not protected by the mutex d.mx, which is used in SubscribeDashbordK8Resources. This is a data race. Please use the copy-under-lock pattern to ensure thread safety.

d.mx.Lock() subscribers := make([]chan struct{}, len(d.ResourcesChan)) copy(subscribers, d.ResourcesChan) d.mx.Unlock() for _, ch := range subscribers { select { case ch <- struct{}{}: default: } }

gemini-code-assist · 2026-05-21T10:00:39Z

 	for _, ch := range c.channel {
-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+		select {
+		case ch <- struct{}{}:
+		default:
 		}
 	}


The iteration over c.channel is not protected by the mutex c.mx, which is used in Subscribe. This is a data race. Please use the copy-under-lock pattern to ensure thread safety.

c.mx.Lock() subscribers := make([]chan struct{}, len(c.channel)) copy(subscribers, c.channel) c.mx.Unlock() for _, ch := range subscribers { select { case ch <- struct{}{}: default: } }

Amr-Shams

just few chagnes and LGTM

Amr-Shams · 2026-05-21T12:11:37Z

@@ -25,8 +23,9 @@ func (c *SummaryChannel) Subscribe(ch chan struct{}) {

 func (c *SummaryChannel) Publish() {
 	for _, ch := range c.channel {


you need to make sure of using mutex here

Done in 181f7da. Copy-under-lock applied to all 5 Publish sites.

Amr-Shams · 2026-05-21T12:12:05Z

@@ -29,8 +27,9 @@ func (c *ConfigurationChannel) SubscribeApplications(ch chan struct{}) {

 func (c *ConfigurationChannel) PublishApplications() {
 	for _, ch := range c.ApplicationsChannel {
-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+		select {


Done in 181f7da. Copy-under-lock applied to all 5 Publish sites.

hamza-mohd · 2026-05-21T16:23:23Z

Smart fix - replacing the IsClosed gate with a non-blocking select correctly handles buffered channels without dropping queued values. Clean, focused change.

@Amr-Shams

Per @Amr-Shams and gemini-code-assist review on PR meshery#19580. The Publish* methods iterated their subscriber slices without holding mx, while the corresponding Subscribe* methods modified them under mx.Lock(). A reallocating append concurrent with the publish loop would race on the slice header and risk reading from a stale backing array. Apply the copy-under-lock pattern already used by PublishContext in server/models/k8scontext_helper.go: c.mx.Lock() subscribers := make([]chan struct{}, len(c.channel)) copy(subscribers, c.channel) c.mx.Unlock() for _, ch := range subscribers { select { case ch <- struct{}{}: default: } } Sites updated: - server/models/configuration_helper.go: PublishApplications, PublishPatterns, PublishFilters - server/models/dashboard_k8s_resources.go: PublishDashboardK8sResources - server/models/meshmodel/helper.go: SummaryChannel.Publish PublishContext in k8scontext_helper.go was already correct and is unchanged. Signed-off-by: SarthakB11 <[email protected]>

Amr-Shams · 2026-05-22T12:29:57Z

-		if !utils.IsClosed(ch) {
-			ch <- struct{}{}
+	c.mx.Lock()
+	subscribers := make([]chan struct{}, len(c.ApplicationsChannel))


why would you do that ?
there is a trade off i can see, copy and process in parraell without blocking the parent, or zero copy but block the caller.
but why one over the other here?

Matches PublishContext in k8scontext_helper.go, so the four siblings line up with that one. Lock-during-iteration would serialize every Subscribe behind the publish for no real gain; the snapshot keeps the critical section to a slice copy and lets new subscribers register concurrently.

hamza-mohd · 2026-05-22T23:13:12Z

Nice concurrency fix. A brief description of the specific race condition being addressed would help reviewers understand the full impact.

hamza-mohd · 2026-05-22T23:13:43Z

Good follow-through on the IsClosed cleanup. Please confirm no goroutine leaks under load (a quick -race run or pprof snapshot would be reassuring).

SarthakB11 · 2026-05-23T10:11:37Z

The "## Three concurrency bugs..." section in the PR body has the full bug breakdown. On goroutine leaks: none of the 4 helper types here have an unsubscribe path that closes channels (grep -rn "close(" server/models/{configuration,dashboard,meshmodel,k8scontext}*.go is empty), so there is no leak vector introduced by the gate removal. The non-blocking select also can't block a publisher goroutine.

SarthakB11 · 2026-07-10T05:56:07Z

Already on master — Lee Calcote's a03fd9a (#20375) did the same IsClosed-gate removal plus non-blocking sends. Closing as superseded.

github-actions Bot added the component/server label May 21, 2026

gemini-code-assist Bot reviewed May 21, 2026

View reviewed changes

SarthakB11 mentioned this pull request May 21, 2026

[server] NewConfigurationHelper and NewSummaryHelper pre-fill subscriber slices with nil channels #19581

Closed

Amr-Shams reviewed May 21, 2026

View reviewed changes

SarthakB11 mentioned this pull request May 21, 2026

[server] Remove utils.IsClosed[K] helper now unused #19585

Closed

1 task done

Amr-Shams reviewed May 22, 2026

View reviewed changes

SarthakB11 mentioned this pull request May 23, 2026

[server] init subscriber slices with len=0 in helper constructors #19643

Closed

1 task done

SarthakB11 closed this Jul 10, 2026

		@@ -25,8 +23,9 @@ func (c *SummaryChannel) Subscribe(ch chan struct{}) {

		func (c *SummaryChannel) Publish() {
		for _, ch := range c.channel {

Conversation

SarthakB11 commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Why non-blocking

Uh oh!

welcome Bot commented May 21, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

Uh oh!

SarthakB11 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Amr-Shams left a comment

Choose a reason for hiding this comment

Uh oh!

Amr-Shams May 21, 2026

Choose a reason for hiding this comment

Uh oh!

SarthakB11 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Amr-Shams May 21, 2026

Choose a reason for hiding this comment

Uh oh!

SarthakB11 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

hamza-mohd commented May 21, 2026

Uh oh!

Amr-Shams May 22, 2026

Choose a reason for hiding this comment

Uh oh!

SarthakB11 May 22, 2026

Choose a reason for hiding this comment

Uh oh!

hamza-mohd commented May 22, 2026

Uh oh!

hamza-mohd commented May 22, 2026

Uh oh!

SarthakB11 commented May 23, 2026

Uh oh!

SarthakB11 commented Jul 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SarthakB11 commented May 21, 2026 •

edited

Loading