From 22fd09692eb943c194d17d65d91cf8cd88ff68e6 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 13 Oct 2026 15:45:55 +0100 Subject: [PATCH 1/3] Make a defensive copy of cmap items during iteration --- src/cmap/cerrmap.go | 6 ++++-- src/cmap/cmap.go | 35 ++++++++++++++++++++++++----------- src/cmap/cmap_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/cmap/cerrmap.go b/src/cmap/cerrmap.go index 16694f674..4c99768aa 100644 --- a/src/cmap/cerrmap.go +++ b/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() { diff --git a/src/cmap/cmap.go b/src/cmap/cmap.go index abc2f47da..4719e313e 100644 --- a/src/cmap/cmap.go +++ b/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() diff --git a/src/cmap/cmap_test.go b/src/cmap/cmap_test.go index c4985fcb1..3023b4413 100644 --- a/src/cmap/cmap_test.go +++ b/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++ { From e4bde31340045241aa517af388331c3a490b8fdc Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Tue, 13 Oct 2026 15:47:00 +0100 Subject: [PATCH 2/3] version --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index e71c09b90..ab2eec2b6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -17.34.0-beta.4 +17.34.0-beta.5 From 5e3b595680bfc57b05c2acdff3bcbf6c03cafa41 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Thu, 15 Oct 2026 11:19:18 +0100 Subject: [PATCH 3/3] Add a minor log message --- src/plz/plz.go | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plz/plz.go b/src/plz/plz.go index c63e1a06b..4571e805d 100644 --- a/src/plz/plz.go +++ b/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.