fix: prevent deadlock in donNotifier.Subscribe() on concurrent NotifyDonSet#21963
Closed
fix: prevent deadlock in donNotifier.Subscribe() on concurrent NotifyDonSet#21963
Conversation
…DonSet Subscribe() used a blocking channel send to deliver the cached DON value. If NotifyDonSet() concurrently filled the buffer between subscriber registration and the send, Subscribe() would deadlock. Switch to a non-blocking select, matching NotifyDonSet()'s pattern. The subscriber already has the value from NotifyDonSet if the buffer is full. Fixes: CORE-2378
product-security-plaid-production
bot
requested review from
DylanTinianov and
archseer
Contributor
|
👋 Fletch153, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
|
|
Collaborator
Author
|
/rerun |
…DonSet Subscribe() registered the subscriber channel in the map BEFORE sending the cached DON value. If NotifyDonSet fired between registration and the send, it would fill the 1-buffer, and the subsequent blocking send in Subscribe() would deadlock. Reorder: send the cached value first (safe — channel is new, buffer is empty, nobody else has a reference), then register. Eliminates the race window entirely without changing the blocking send contract. Fixes: CORE-2378
mchain0
previously approved these changes
Apr 13, 2026
jmank88
requested changes
Apr 13, 2026
Contributor
There was a problem hiding this comment.
This change is not as described. Does it actually fix the issue?
Register the subscriber first (preserving no-missed-notification guarantee), but use a non-blocking select when sending the cached value. If a concurrent NotifyDonSet already filled the buffer, the cached send is safely skipped since the subscriber already has a notification. Also fixes a minor double-Load race in the original code by capturing the pointer once.
|
Collaborator
Author
|
Closing issue - required further investigation |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a deadlock in
Subscribe()that occurs whenNotifyDonSet()fires concurrently.The channel returned by
Subscribe()has a buffer of 1. In the original code,Subscribe()registered the subscriber in the map before sending the cached DON value with a blocking send. IfNotifyDonSet()ran between those two steps, it would fill the buffer via its non-blockingselectsend, and then the blockings <- *n.don.Load()inSubscribe()would block forever — the caller has not received the channel yet, so nobody is reading from it.Root Cause
Race window in the original
Subscribe():n.subscribers.Store(s, struct{}{})— subscriber visible toNotifyDonSetNotifyDonSet()runs concurrently: stores new DON, iterates subscribers, non-blocking send fills the 1-capacity buffers <- *n.don.Load()— blocking send on a now-full buffer, no reader exists yet — deadlockNote:
NotifyDonSet()itself uses a non-blockingselectand cannot deadlock. The deadlock is inSubscribe()'s own blocking send, not inNotifyDonSet().Fix
Change the cached-value send in
Subscribe()to a non-blockingselect, matching the pattern already used inNotifyDonSet(). Registration stays first to guarantee no missed notifications. If a concurrentNotifyDonSetalready filled the buffer, the cached send is safely skipped — the subscriber already has a value queued.Test plan
TestDonNotifierpasses with-count=100 -race#bugfix
Fixes: CORE-2378