Make a copy of map items during iteration so it's threadsafe

VERSION

@@ -1 +1 @@
-17.34.0-beta.4
+17.34.0-beta.5

src/cmap/cerrmap.go

@@ -138,7 +138,8 @@ func (m *ErrMap[K, V]) GetOrWait(key K) (V, <-chan struct{}, bool, error) {
 
 // Items returns an iterator over all key / value pairs in the map, which have not errored.
 // They are returned in no particular order.
-// You should not mutate the map while calling this as it may deadlock.
+// The caller is allowed to modify the map during iteration, although no guarantee is made about whether those
+// modifications will become visible during this iteration.
 func (m *ErrMap[K, V]) Items() iter.Seq2[K, V] {
 	return func(yield func(K, V) bool) {
 		for k, v := range m.m.Items() {
@@ -153,7 +154,8 @@ func (m *ErrMap[K, V]) Items() iter.Seq2[K, V] {
 
 // Values returns an iterator over all value / error pairs in the map.
 // They are returned in no particular order.
-// You should not mutate the map while calling this as it may deadlock.
+// The caller is allowed to modify the map during iteration, although no guarantee is made about whether those
+// modifications will become visible during this iteration.
 func (m *ErrMap[K, V]) Values() iter.Seq2[V, error] {
 	return func(yield func(V, error) bool) {
 		for _, v := range m.m.Items() {

src/cmap/cmap.go

@@ -94,22 +94,17 @@ func (m *Map[K, V]) Values() []V {
 }
 
 // Items returns an iterator over each key-value pair in the map, not including anything that is still waiting to be inserted.
-// They are returned in no particular order.
-// You should not mutate the map while calling this as it may deadlock.
+// They are returned in no particular order. No particular consistency guarantees are made.
+// The caller is allowed to modify the map during iteration, although no guarantee is made about whether those
+// modifications will become visible during this iteration.
 func (m *Map[K, V]) Items() iter.Seq2[K, V] {
 	return func(yield func(K, V) bool) {
 		for i := range len(m.shards) {
-			shard := &m.shards[i]
-			shard.l.RLock()
-			for k, v := range shard.m {
-				if v.Wait == nil {
-					if !yield(k, v.Val) {
-						shard.l.RUnlock()
-						return
-					}
+			for _, item := range m.shards[i].Items() {
+				if !yield(item.Key, item.Val) {
+					return
 				}
 			}
-			shard.l.RUnlock()
 		}
 	}
 }
@@ -196,6 +191,24 @@ func (s *shard[K, V]) Update(key K, update func(V) V) V {
 	return newv
 }
 
+type item[K comparable, V any] struct {
+	Key K
+	Val V
+}
+
+// Items returns a copy of all the items currently in the shard (not including those that are not complete yet).
+func (s *shard[K, V]) Items() []item[K, V] {
+	s.l.RLock()
+	defer s.l.RUnlock()
+	ret := make([]item[K, V], 0, len(s.m))
+	for k, v := range s.m {
+		if v.Wait == nil {
+			ret = append(ret, item[K, V]{Key: k, Val: v.Val})
+		}
+	}
+	return ret
+}
+
 // Values returns a copy of all the targets currently in the map.
 func (s *shard[K, V]) Values() []V {
 	s.l.RLock()

src/cmap/cmap_test.go

@@ -5,6 +5,7 @@ import (
 	"sort"
 	"strconv"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/sync/errgroup"
@@ -77,6 +78,32 @@ func TestUpdate(t *testing.T) {
 	assert.Equal(t, 3, m.Get(1))
 }
 
+// TestItemsAccessDuringIteration checks that reading the map while iterating it doesn't deadlock against a concurrent writer.
+func TestItemsAccessDuringIteration(t *testing.T) {
+	m := New[int, int](1, hashInts) // Single shard so everything collides
+	m.Add(1, 1)
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		for k := range m.Items() {
+			updated := make(chan struct{})
+			go func() {
+				m.Update(2, func(int) int { return 2 })
+				close(updated)
+			}()
+			time.Sleep(50 * time.Millisecond) // Give the writer time to queue up on the shard's lock
+			m.Get(k)
+			<-updated
+		}
+	}()
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("deadlocked accessing map during iteration")
+	}
+	assert.Equal(t, 2, m.Get(2))
+}
+
 func BenchmarkMapInserts(b *testing.B) {
 	m := New[int, int](DefaultShardCount, hashInts)
 	for i := 0; i < b.N; i++ {

src/plz/plz.go

@@ -41,6 +41,7 @@ func Run(targets, preTargets []core.BuildLabel, state *core.BuildState, progress
 	if state.Config.Display.SystemStats {
 		go state.UpdateResources()
 	}
+	log.Debug("Beginning build, local executors: %d remote executors: %d", state.Config.Please.NumThreads, state.Config.NumRemoteExecutors())
 
 	// This must happen however we exit; anything reading state.Results() (e.g. the display)
 	// waits for that channel to be closed, so it would hang forever if we returned an error first.