Skip to content

sync: fix deadlock when a Map method is called from Map.Range#5529

Open
fxamacker wants to merge 1 commit into
tinygo-org:devfrom
fxamacker:fix-syncmap-range-deadlock
Open

sync: fix deadlock when a Map method is called from Map.Range#5529
fxamacker wants to merge 1 commit into
tinygo-org:devfrom
fxamacker:fix-syncmap-range-deadlock

Conversation

@fxamacker

@fxamacker fxamacker commented Jul 15, 2026

Copy link
Copy Markdown

Fixes #5528

Currently, calling any sync.Map method from 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 PR prevents the deadlock by changing sync.Map.Range to:

  • take a snapshot of the map 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 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.

Range likewise holds the lock across the user callback, so any sync.Map method called from inside the callback deadlocks.

Added Tests

Added TestMapRangeAndDelete regression test, which deletes map entries from inside the map's Range callback.

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

  • Updated to manually copy entries instead of using maps.Clone. Initially, this PR used maps.Clone, but importing maps from sync triggers an import cycle not allowed error on baremetal targets (e.g. -target=microbit) and breaks make smoketest.

@dgryski

dgryski commented Jul 15, 2026

Copy link
Copy Markdown
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?

@fxamacker

Copy link
Copy Markdown
Author

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?

@dgryski yes, we can snapshot just the keys under the lock and then Load each value from the m. However, it has different tradeoffs.

Pros:

  • Less memory (snapshot only keys, instead of keys + values)

Cons:

  • Instead of 1 lock acquisition, N+1 lock acquisitions (1 for the keys, then one per Load). Many more lock operations, but shorter individual holds.
  • Not a consistent point-in-time view of the map: values can be changed and keys can be deleted between snapshot of the key and loading the value.

Both approaches satisfy Go's sync.Map.Range contract. As noted in the Tradeoffs section, I went with maps.Clone to prioritize simplicity, predictability, and minimal changes to existing code.

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
fxamacker force-pushed the fix-syncmap-range-deadlock branch from 1afc555 to 75f3342 Compare July 16, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sync: deadlock when calling any sync.Map method inside Map.Range callback

2 participants