sync: fix deadlock when a Map method is called from Map.Range#5529
Open
fxamacker wants to merge 1 commit into
Open
sync: fix deadlock when a Map method is called from Map.Range#5529fxamacker wants to merge 1 commit into
fxamacker wants to merge 1 commit into
Conversation
Member
|
Do we need to clone the entire map or can we get away with just getting a copy of all the keys and ranging over those and looking up values in the original map? |
Author
@dgryski yes, we can snapshot just the keys under the lock and then Load each value from the Pros:
Cons:
Both approaches satisfy Go's Happy to switch to the key-snapshot approach if you prefer. Please let me know. |
Currently, calling any sync.Map method from inside the sync.Map.Range
callback f deadlocks. Moreover, Go's sync.Map explicitly permits the
Range callback to call other methods on the map ("Range does not
block other methods on the receiver; even f itself may call any
method on m").
This commit prevents the deadlock by changing sync.Map.Range to:
- copy the map's entries under the lock
- release the lock
- iterate over the snapshot
A snapshot satisfies Go's sync.Map.Range contract, which only
requires that no key is visited more than once and may reflect any
mapping from any point during the call.
Using a snapshot keeps the implementation simple, in line with this
file's stated scope ("no more efficient than a map with a lock").
Also added TestMapRangeAndDelete regression test, which deletes map
entries from inside the map's Range callback.
fxamacker
force-pushed
the
fix-syncmap-range-deadlock
branch
from
July 16, 2026 14:33
1afc555 to
75f3342
Compare
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
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.
Fixes #5528
Currently, calling any
sync.Mapmethod fromsync.Map.Rangecallbackfdeadlocks. Moreover, Go'ssync.Mapexplicitly permits theRangecallback to call other methods on the map ("Range does not block other methods on the receiver; even f itself may call any method on m").This PR prevents the deadlock by changing
sync.Map.Rangeto:A snapshot satisfies Go's
sync.Map.Rangecontract, which only requires that no key is visited more than once and may reflect any mapping from any point during the call.Using a snapshot keeps the implementation simple with minimal changes to existing code, in line with this file's stated scope ("no more efficient than a map with a lock").
Deadlock Details
src/sync/map.go is a plain map guarded by a single non-reentrant
task.PMutex, and every method holds that lock for its entire duration.Rangelikewise holds the lock across the user callback, so anysync.Mapmethod called from inside the callback deadlocks.Added Tests
Added
TestMapRangeAndDeleteregression test, which deletes map entries from inside the map'sRangecallback.Tradeoffs
The snapshot approach prioritized simplicity, predictability, and minimal changes to existing code over performance.
See comment about copying entire map vs copying just the keys.
PR Updates
maps.Clone. Initially, this PR usedmaps.Clone, but importingmapsfromsynctriggers animport cycle not allowederror on baremetal targets (e.g.-target=microbit) and breaksmake smoketest.