sync: fix deadlock in Map.Range callback

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 keys under the lock
- release the lock
- iterate over the key 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.

Author: fxamacker

src/sync/map.go

@@ -1,6 +1,8 @@
 package sync
 
-import "internal/task"
+import (
+	"internal/task"
+)
 
 // This file implements just enough of sync.Map to get packages to compile. It
 // is no more efficient than a map with a lock.
@@ -56,17 +58,27 @@ func (m *Map) Store(key, value interface{}) {
 	m.m[key] = value
 }
 
+// Range calls f for each key and value in the map.  If f returns false, the iteration stops.
 func (m *Map) Range(f func(key, value interface{}) bool) {
-	m.lock.Lock()
-	defer m.lock.Unlock()
+	// Iterate over a key snapshot instead of holding the lock across the callback,
+	// to prevent deadlock when a Map method is called inside f.
+	//
+	// Using a key snapshot in Map.Range is sufficient because Go specifies that:
+	// - Range only requires that no key is visited more than once, and
+	// - Range may reflect any mapping from any point during the Range call.
 
-	if m.m == nil {
-		return
+	m.lock.Lock()
+	keys := make([]interface{}, 0, len(m.m))
+	for k := range m.m {
+		keys = append(keys, k)
 	}
+	m.lock.Unlock()
 
-	for k, v := range m.m {
-		if !f(k, v) {
-			break
+	for _, k := range keys {
+		if v, ok := m.Load(k); ok {
+			if !f(k, v) {
+				break
+			}
 		}
 	}
 }

src/sync/map_test.go

@@ -36,3 +36,31 @@ func TestMapSwap(t *testing.T) {
 		t.Errorf("Load after Swap returned %v, %v, want foo, true", v, ok)
 	}
 }
+
+func TestMapRangeAndDelete(t *testing.T) {
+	var sm sync.Map
+	sm.Store(0, "0")
+	sm.Store(1, "1")
+	sm.Store(2, "2")
+
+	sm.Range(func(k, v any) bool {
+		keyAsInt, ok := k.(int)
+		if !ok {
+			return true
+		}
+		if keyAsInt%2 == 0 {
+			sm.Delete(keyAsInt)
+		}
+		return true
+	})
+
+	if v, ok := sm.Load(0); ok {
+		t.Errorf("Load(0) after Delete returned %v, %v, want nil, false", v, ok)
+	}
+	if v, ok := sm.Load(1); !ok || v.(string) != "1" {
+		t.Errorf("Load(1) after Delete returned %v, %v, want \"1\", true", v, ok)
+	}
+	if v, ok := sm.Load(2); ok {
+		t.Errorf("Load(2) after Delete returned %v, %v, want nil, false", v, ok)
+	}
+}