diff --git a/selection/selection.go b/selection/selection.go index c537dda..5507535 100644 --- a/selection/selection.go +++ b/selection/selection.go @@ -37,11 +37,24 @@ func (s *Set) Add(l line.Line) { } // Copy copies all selected lines from s into dst. +// +// Items are collected under the source read lock first, then added to dst +// after releasing that lock. This avoids deadlocks when dst == s and +// prevents ABBA deadlocks during concurrent bidirectional copies. func (s *Set) Copy(dst *Set) { - s.Ascend(func(l line.Line) bool { - dst.Add(l) + s.mutex.RLock() + items := make([]line.Line, 0, s.tree.Len()) + s.tree.Ascend(func(it btree.Item) bool { + if l, ok := it.(line.Line); ok { + items = append(items, l) + } return true }) + s.mutex.RUnlock() + + for _, l := range items { + dst.Add(l) + } } // Remove removes the specified line from the selection. diff --git a/selection/selection_test.go b/selection/selection_test.go index 7b17319..9bc6cf7 100644 --- a/selection/selection_test.go +++ b/selection/selection_test.go @@ -3,6 +3,7 @@ package selection import ( "sync" "testing" + "time" "github.com/peco/peco/line" "github.com/stretchr/testify/require" @@ -110,3 +111,52 @@ func TestRangeStart(t *testing.T) { rs.Reset() require.False(t, rs.Valid()) } + +func TestCopySelf(t *testing.T) { + s := New() + s.Add(line.NewRaw(0, "Alice", false, false)) + s.Add(line.NewRaw(1, "Bob", false, false)) + + done := make(chan struct{}) + go func() { + defer close(done) + s.Copy(s) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Copy(self) deadlocked") + } + + require.Equal(t, 2, s.Len()) +} + +func TestCopyCrossNoDeadlock(t *testing.T) { + a := New() + b := New() + a.Add(line.NewRaw(0, "Alice", false, false)) + b.Add(line.NewRaw(1, "Bob", false, false)) + + done := make(chan struct{}) + go func() { + defer close(done) + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + a.Copy(b) + }() + go func() { + defer wg.Done() + b.Copy(a) + }() + wg.Wait() + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("cross-Copy deadlocked") + } +}