Fix idle notification deadlock (#5821)

Running the integration tests in a loop under the race detector
eventually hung in demo/bisect. The goroutine dump shows the cycle: a
background worker's task.Done() held the task manager's mutex while
blocking on the unbuffered idle-listener channel send, and the test
runner goroutine — the only reader of that channel — was itself blocked
in NewTask on that same mutex, on its way to enqueueing a caption render
(SetCaption -> Render -> OnUIThread). Neither side could proceed: the
notification couldn't be delivered until the test goroutine got the
mutex, and the mutex couldn't be released until the notification was
delivered.

The root problem is that the busy-to-idle notification is a blocking
rendezvous performed while holding the mutex, so it needs the waiter's
cooperation at a moment where the waiter may legitimately need the mutex
first.

Make the notification fire-and-forget instead: WaitUntilIdle waits on a
condition variable and re-checks "is any task busy?" under the mutex,
and the busy-to-idle transition broadcasts, which never blocks. Waiting
is now level-triggered rather than edge-triggered, which is also more
robust: a wait can no longer be satisfied by a stale idle transition
produced by an unrelated background task, because the predicate is
evaluated against the current state. This relies on the previous commit
having made replayed input events carry their task from submission;
without that, the wait could return in the window where an event is in
flight but not yet picked up by the main loop.

pkg/gocui/gui.go

@@ -125,8 +125,11 @@ type clickInfo struct {
 // and keybindings.
 type Gui struct {
 	RecordingConfig
-	// ReplayedEvents is for passing pre-recorded input events, for the purposes of testing
-	ReplayedEvents replayedEvents
+	// replayedEvents is for passing simulated input events, for the purposes
+	// of testing. Events must be submitted through the Replay* methods, which
+	// attach a task to each event; pushing into the channels directly would
+	// bypass the busy-tracking that integration tests rely on.
+	replayedEvents replayedEvents
 	playRecording  bool
 
 	tabClickBindings         []*tabClickBinding
@@ -255,7 +258,7 @@ func NewGui(opts NewGuiOpts) (*Gui, error) {
 	g.taskManager = newTaskManager()
 
 	if opts.PlayRecording {
-		g.ReplayedEvents = replayedEvents{
+		g.replayedEvents = replayedEvents{
 			Keys:        make(chan *TcellKeyEventWrapper),
 			Resizes:     make(chan *TcellResizeEventWrapper),
 			MouseEvents: make(chan *TcellMouseEventWrapper),
@@ -291,6 +294,30 @@ func (g *Gui) NewBackgroundTask() *TaskImpl {
 	return g.taskManager.NewTask(true)
 }
 
+// ReplayKeyEvent simulates a key press, as if the user had typed it. It's used
+// by integration tests. The event carries a task, so that the program counts
+// as busy from before the event is submitted until the main loop has fully
+// processed it; the test driver relies on this when it waits for the program
+// to go idle after submitting an event. (If the task were only created once
+// the main loop picks the event up, there would be a window in which the event
+// is still in flight but nothing counts as busy.)
+func (g *Gui) ReplayKeyEvent(ev *TcellKeyEventWrapper) {
+	ev.task = g.NewTask()
+	g.replayedEvents.Keys <- ev
+}
+
+// ReplayMouseEvent is like ReplayKeyEvent, but for mouse events.
+func (g *Gui) ReplayMouseEvent(ev *TcellMouseEventWrapper) {
+	ev.task = g.NewTask()
+	g.replayedEvents.MouseEvents <- ev
+}
+
+// ReplayFocusEvent is like ReplayKeyEvent, but for focus events.
+func (g *Gui) ReplayFocusEvent(ev *TcellFocusEventWrapper) {
+	ev.task = g.NewTask()
+	g.replayedEvents.FocusEvents <- ev
+}
+
 // Busy reports whether any foreground work is in flight, ignoring the event
 // currently being processed on the main goroutine (see currentTask). Background
 // routines (auto-fetch etc.) don't count. It's used to decide whether it's safe
@@ -299,11 +326,11 @@ func (g *Gui) Busy() bool {
 	return g.taskManager.hasBusyForegroundTaskExcept(g.currentTask)
 }
 
-// An idle listener listens for when the program is idle. This is useful for
-// integration tests which can wait for the program to be idle before taking
-// the next step in the test.
-func (g *Gui) AddIdleListener(c chan struct{}) {
-	g.taskManager.addIdleListener(c)
+// WaitUntilIdle blocks until the program is idle (no busy tasks). This is
+// useful for integration tests which want to wait for the program to finish
+// processing before taking the next step in the test.
+func (g *Gui) WaitUntilIdle() {
+	g.taskManager.WaitUntilIdle()
 }
 
 // Close finalizes the library. It should be called after a successful
@@ -948,7 +975,12 @@ func (g *Gui) processEvent() error {
 	// are always the primary event here.
 	select {
 	case ev := <-g.gEvents:
-		task := g.NewTask()
+		// Replayed test events already carry their task (see ReplayKeyEvent);
+		// organic events get theirs here.
+		task := ev.task
+		if task == nil {
+			task = g.NewTask()
+		}
 		g.currentTask = task
 		defer func() { g.currentTask = nil; task.Done() }()
 
@@ -992,7 +1024,11 @@ func (g *Gui) processRemainingEvents() (bool, error) {
 		select {
 		case ev := <-g.gEvents:
 			contentOnly = false
-			if err := g.handleError(g.handleEvent(&ev)); err != nil {
+			err := g.handleError(g.handleEvent(&ev))
+			if ev.task != nil {
+				ev.task.Done()
+			}
+			if err != nil {
 				return false, err
 			}
 		default:

pkg/gocui/task_manager.go

@@ -6,20 +6,23 @@ import "sync"
 // the main goroutine or a worker goroutine). Used by integration tests
 // to wait until the program is idle before progressing.
 type TaskManager struct {
-	// each of these listeners will be notified when the program goes from busy to idle
-	idleListeners []chan struct{}
-	tasks         map[int]Task
+	tasks map[int]Task
 	// auto-incrementing id for new tasks
 	nextId int
 
 	mutex sync.Mutex
+	// signalled whenever the program transitions from busy to idle; used by
+	// WaitUntilIdle
+	idleCond *sync.Cond
 }
 
 func newTaskManager() *TaskManager {
-	return &TaskManager{
-		tasks:         make(map[int]Task),
-		idleListeners: []chan struct{}{},
+	self := &TaskManager{
+		tasks: make(map[int]Task),
 	}
+	self.idleCond = sync.NewCond(&self.mutex)
+
+	return self
 }
 
 func (self *TaskManager) NewTask(background bool) *TaskImpl {
@@ -58,8 +61,26 @@ func (self *TaskManager) hasBusyForegroundTaskExcept(ignore Task) bool {
 	return false
 }
 
-func (self *TaskManager) addIdleListener(c chan struct{}) {
-	self.idleListeners = append(self.idleListeners, c)
+// WaitUntilIdle blocks until no task is busy. Integration tests use it to wait
+// for the program to finish processing before taking the next step.
+func (self *TaskManager) WaitUntilIdle() {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	for self.hasBusyTask() {
+		self.idleCond.Wait()
+	}
+}
+
+// caller must hold self.mutex
+func (self *TaskManager) hasBusyTask() bool {
+	for _, task := range self.tasks {
+		if task.isBusy() {
+			return true
+		}
+	}
+
+	return false
 }
 
 func (self *TaskManager) withMutex(f func()) {
@@ -68,17 +89,12 @@ func (self *TaskManager) withMutex(f func()) {
 
 	f()
 
-	// Check if all tasks are done
-	for _, task := range self.tasks {
-		if task.isBusy() {
-			return
-		}
-	}
-
-	// If we get here, all tasks are done, so
-	// notify listeners that the program is idle
-	for _, listener := range self.idleListeners {
-		listener <- struct{}{}
+	// Wake up any goroutine blocked in WaitUntilIdle. This must not block on
+	// the waiter (we hold the mutex, and the waiter may itself be trying to
+	// acquire it, e.g. by creating a task, before it next waits) — which is
+	// exactly what Broadcast guarantees.
+	if !self.hasBusyTask() {
+		self.idleCond.Broadcast()
 	}
 }
 

pkg/gocui/task_manager_test.go

@@ -2,6 +2,7 @@ package gocui
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -61,3 +62,68 @@ func TestTaskManagerHasBusyForegroundTaskExcept(t *testing.T) {
 		assert.False(t, tm.hasBusyForegroundTaskExcept(current))
 	})
 }
+
+func TestTaskManagerWaitUntilIdle(t *testing.T) {
+	// returnsWithin reports whether f returns within the given duration.
+	returnsWithin := func(d time.Duration, f func()) bool {
+		done := make(chan struct{})
+		go func() {
+			f()
+			close(done)
+		}()
+		select {
+		case <-done:
+			return true
+		case <-time.After(d):
+			return false
+		}
+	}
+
+	t.Run("returns immediately when no task was ever created", func(t *testing.T) {
+		tm := newTaskManager()
+		assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle))
+	})
+
+	t.Run("blocks while a task is busy", func(t *testing.T) {
+		tm := newTaskManager()
+		tm.NewTask(false)
+		assert.False(t, returnsWithin(50*time.Millisecond, tm.WaitUntilIdle))
+	})
+
+	t.Run("wakes up when the last busy task completes", func(t *testing.T) {
+		tm := newTaskManager()
+		task := tm.NewTask(false)
+		go func() {
+			time.Sleep(10 * time.Millisecond)
+			task.Done()
+		}()
+		assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle))
+	})
+
+	t.Run("a paused task counts as idle", func(t *testing.T) {
+		tm := newTaskManager()
+		task := tm.NewTask(false)
+		task.Pause()
+		assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle))
+	})
+
+	t.Run("a task completing while nobody waits must not block", func(t *testing.T) {
+		// This is the deadlock case: the waiter (the integration-test runner)
+		// is between waits, and itself needs the task manager's mutex (it
+		// creates a task whenever it enqueues work) before it waits again. The
+		// idle notification must neither block the completing task while it
+		// holds the mutex, nor get lost.
+		tm := newTaskManager()
+		assert.True(t, returnsWithin(time.Second, func() {
+			// the program goes idle with nobody waiting...
+			tm.NewTask(true).Done()
+
+			// ...and creating and completing more tasks afterwards must still
+			// be possible
+			task := tm.NewTask(false)
+			tm.NewTask(false).Done()
+			task.Done()
+		}))
+		assert.True(t, returnsWithin(time.Second, tm.WaitUntilIdle))
+	})
+}

pkg/gocui/tcell_driver.go

@@ -172,6 +172,12 @@ type GocuiEvent struct {
 	Focused bool
 	Start   bool
 	N       int
+
+	// task tracks the processing of this event for idle detection. Events
+	// replayed by integration tests carry a task from the moment they are
+	// submitted (see Gui.ReplayKeyEvent); for organic events it is nil, and
+	// the main loop creates a task when it picks the event up.
+	task Task
 }
 
 // Event types.
@@ -208,6 +214,8 @@ type TcellKeyEventWrapper struct {
 	Mod       tcell.ModMask
 	Key       tcell.Key
 	Ch        string
+
+	task Task // see GocuiEvent.task
 }
 
 func NewTcellKeyEventWrapper(event *tcell.EventKey, timestamp int64) *TcellKeyEventWrapper {
@@ -229,6 +237,8 @@ type TcellMouseEventWrapper struct {
 	Y          int
 	ButtonMask tcell.ButtonMask
 	ModMask    tcell.ModMask
+
+	task Task // see GocuiEvent.task
 }
 
 func NewTcellMouseEventWrapper(event *tcell.EventMouse, timestamp int64) *TcellMouseEventWrapper {
@@ -269,6 +279,8 @@ func (wrapper TcellResizeEventWrapper) toTcellEvent() tcell.Event {
 type TcellFocusEventWrapper struct {
 	Timestamp int64
 	Focused   bool
+
+	task Task // see GocuiEvent.task
 }
 
 func NewTcellFocusEventWrapper(event *tcell.EventFocus, timestamp int64) *TcellFocusEventWrapper {
@@ -285,21 +297,31 @@ func (wrapper TcellFocusEventWrapper) toTcellEvent() tcell.Event {
 // pollEvent get tcell.Event and transform it into gocuiEvent
 func (g *Gui) pollEvent() GocuiEvent {
 	var tev tcell.Event
+	var task Task
 	if g.playRecording {
 		select {
-		case ev := <-g.ReplayedEvents.Keys:
+		case ev := <-g.replayedEvents.Keys:
 			tev = (ev).toTcellEvent()
-		case ev := <-g.ReplayedEvents.Resizes:
+			task = ev.task
+		case ev := <-g.replayedEvents.Resizes:
 			tev = (ev).toTcellEvent()
-		case ev := <-g.ReplayedEvents.MouseEvents:
+		case ev := <-g.replayedEvents.MouseEvents:
 			tev = (ev).toTcellEvent()
-		case ev := <-g.ReplayedEvents.FocusEvents:
+			task = ev.task
+		case ev := <-g.replayedEvents.FocusEvents:
 			tev = (ev).toTcellEvent()
+			task = ev.task
 		}
 	} else {
 		tev = <-Screen.EventQ()
 	}
 
+	event := gocuiEventFromTcellEvent(tev)
+	event.task = task
+	return event
+}
+
+func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent {
 	switch tev := tev.(type) {
 	case *tcell.EventInterrupt:
 		return GocuiEvent{Type: eventInterrupt}

pkg/gui/gui_driver.go

@@ -17,10 +17,9 @@ import (
 // this gives our integration test a way of interacting with the gui for sending keypresses
 // and reading state.
 type GuiDriver struct {
-	gui        *Gui
-	isIdleChan chan struct{}
-	toastChan  chan string
-	headless   bool
+	gui       *Gui
+	toastChan chan string
+	headless  bool
 }
 
 var _ integrationTypes.GuiDriver = &GuiDriver{}
@@ -33,10 +32,10 @@ func (self *GuiDriver) PressKey(keyStr string) {
 		self.Fail("Unrecognized key: " + keyStr)
 	}
 
-	self.gui.g.ReplayedEvents.Keys <- gocui.NewTcellKeyEventWrapper(
+	self.gui.g.ReplayKeyEvent(gocui.NewTcellKeyEventWrapper(
 		tcell.NewEventKey(tcell.Key(key.KeyName()), key.Str(), tcell.ModMask(key.Mod())),
 		0,
-	)
+	))
 
 	self.waitTillIdle()
 }
@@ -44,15 +43,15 @@ func (self *GuiDriver) PressKey(keyStr string) {
 func (self *GuiDriver) Click(x, y int) {
 	self.CheckAllToastsAcknowledged()
 
-	self.gui.g.ReplayedEvents.MouseEvents <- gocui.NewTcellMouseEventWrapper(
+	self.gui.g.ReplayMouseEvent(gocui.NewTcellMouseEventWrapper(
 		tcell.NewEventMouse(x, y, tcell.ButtonPrimary, 0),
 		0,
-	)
+	))
 	self.waitTillIdle()
-	self.gui.g.ReplayedEvents.MouseEvents <- gocui.NewTcellMouseEventWrapper(
+	self.gui.g.ReplayMouseEvent(gocui.NewTcellMouseEventWrapper(
 		tcell.NewEventMouse(x, y, tcell.ButtonNone, 0),
 		0,
-	)
+	))
 	self.waitTillIdle()
 }
 
@@ -60,10 +59,10 @@ func (self *GuiDriver) Click(x, y int) {
 // learns to reload changed config files. Tests use it to exercise the live
 // config-reload path.
 func (self *GuiDriver) FocusIn() {
-	self.gui.g.ReplayedEvents.FocusEvents <- gocui.NewTcellFocusEventWrapper(
+	self.gui.g.ReplayFocusEvent(gocui.NewTcellFocusEventWrapper(
 		tcell.NewEventFocus(true),
 		0,
-	)
+	))
 
 	self.waitTillIdle()
 }
@@ -79,7 +78,7 @@ func (self *GuiDriver) PretendMergeOrRebaseStartedInLazygit() {
 
 // wait until lazygit is idle (i.e. all processing is done) before continuing
 func (self *GuiDriver) waitTillIdle() {
-	<-self.isIdleChan
+	self.gui.g.WaitUntilIdle()
 }
 
 func (self *GuiDriver) CheckAllToastsAcknowledged() {

pkg/gui/test_mode.go

@@ -23,12 +23,8 @@ func (gui *Gui) handleTestMode() {
 	}
 
 	if test != nil {
-		isIdleChan := make(chan struct{})
-
-		gui.c.GocuiGui().AddIdleListener(isIdleChan)
-
 		waitUntilIdle := func() {
-			<-isIdleChan
+			gui.c.GocuiGui().WaitUntilIdle()
 		}
 
 		go func() {
@@ -38,7 +34,7 @@ func (gui *Gui) handleTestMode() {
 			gui.PopupHandler.(*popup.PopupHandler).SetToastFunc(
 				func(message string, kind types.ToastKind) { toastChan <- message })
 
-			test.Run(&GuiDriver{gui: gui, isIdleChan: isIdleChan, toastChan: toastChan, headless: Headless()})
+			test.Run(&GuiDriver{gui: gui, toastChan: toastChan, headless: Headless()})
 
 			gui.g.Update(func(*gocui.Gui) error {
 				return gocui.ErrQuit