Fix data race on cancelFunc and err fields

peco.go

@@ -253,6 +253,8 @@ func (p *Peco) Hub() MessageHub {
 }
 
 func (p *Peco) Err() error {
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
 	return p.err
 }
 
@@ -261,8 +263,11 @@ func (p *Peco) Exit(err error) {
 		g := pdebug.Marker("Peco.Exit (err = %s)", err)
 		defer g.End()
 	}
+	p.mutex.Lock()
 	p.err = err
-	if cf := p.cancelFunc; cf != nil {
+	cf := p.cancelFunc
+	p.mutex.Unlock()
+	if cf != nil {
 		cf()
 	}
 }
@@ -445,8 +450,10 @@ func (p *Peco) Run(ctx context.Context) (err error) {
 	// start the ID generator
 	go p.idgen.Run(ctx)
 
-	// remember this cancel func so p.Exit works (XXX requires locking?)
+	// remember this cancel func so p.Exit works
+	p.mutex.Lock()
 	p.cancelFunc = cancel
+	p.mutex.Unlock()
 
 	sigH := sig.New(sig.SigReceivedHandlerFunc(func(sig os.Signal) {
 		p.Exit(errors.New("received signal: " + sig.String()))

peco_test.go

@@ -903,3 +903,54 @@ func TestQueryExecTimerStoppedOnCancel(t *testing.T) {
 	p.queryExec.mutex.Unlock()
 	require.Nil(t, timerAfterCancel, "queryExec.timer should be nil after cancellation")
 }
+
+// TestCancelFuncDataRace verifies that concurrent calls to Exit() and
+// reads of Err() do not race with Run()'s write to cancelFunc. Without
+// proper mutex protection on p.cancelFunc and p.err, the race detector
+// flags this as a data race.
+func TestCancelFuncDataRace(t *testing.T) {
+	p := newPeco()
+	p.Stdin = bytes.NewBufferString("foo\nbar\nbaz\n")
+	var out bytes.Buffer
+	p.Stdout = &out
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	waitCh := make(chan error, 1)
+	go func() {
+		waitCh <- p.Run(ctx)
+	}()
+
+	// Wait for peco to be ready (cancelFunc has been set by now)
+	<-p.Ready()
+
+	// Launch several goroutines that concurrently call Exit() and Err().
+	// Under the race detector, unprotected access to p.cancelFunc and
+	// p.err would be flagged.
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			_ = p.Err()
+		}()
+	}
+	for i := 0; i < 5; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			p.Exit(fmt.Errorf("exit-%d", i))
+		}(i)
+	}
+	wg.Wait()
+
+	// One of the Exit calls should have cancelled the context.
+	select {
+	case err := <-waitCh:
+		// err could be any of the "exit-N" errors; just verify it's non-nil
+		require.Error(t, err, "Run should return an error after Exit")
+	case <-time.After(5 * time.Second):
+		t.Fatal("timeout waiting for Run to return")
+	}
+}