Fix hang on quit when confirmOnQuit is true (#5919)

When confirmOnQuit is true, quitting would sometimes hang for three
seconds and then print "cannot kill child process". Concretely, this
happened whenever the Files panel was focused but there were no changed
files (the main view shows "No changed files").

This is a regression in 0.64.0, it worked before.

Fixes #5918.

pkg/gocui/gui.go

@@ -38,6 +38,11 @@ var (
 
 	// ErrKeybindingNotHandled is returned when a keybinding is not handled, so that the key can be dispatched further
 	ErrKeybindingNotHandled = standardErrors.New("keybinding not handled")
+
+	// ErrLoopExited is returned by OnUIThreadAndWait when MainLoop has already
+	// returned. Nothing dequeues user events after that, so the callback it was
+	// asked to run on the main goroutine never will be.
+	ErrLoopExited = standardErrors.New("main loop exited")
 )
 
 const (
@@ -893,36 +898,50 @@ func (g *Gui) EndBlockingEvents() error {
 }
 
 // OnUIThreadAndWait runs f on the main event-loop goroutine and blocks the
-// caller until f has run, returning f's error. Use it to read UI-thread-owned
-// state (the model, contexts) from a worker without racing the UI thread.
+// caller until f has run. Use it to read UI-thread-owned state (the model,
+// contexts) from a worker without racing the UI thread.
+//
+// The error it returns is the wait's own, never f's: it reports that f was not
+// run at all, which happens when the main loop has exited (ErrLoopExited). f
+// doesn't report an error because what callers want on the UI thread — reading
+// and mutating state — doesn't fail.
 //
 // It must be called from a worker goroutine, never from the UI thread itself:
 // the UI thread would block waiting for a callback only it can run, which
 // deadlocks. Callers arrange this by construction (see the refresh helper's
 // RefreshFromWorker); a debug-only assertion there guards against getting it
 // wrong.
-func (g *Gui) OnUIThreadAndWait(f func() error) error {
+func (g *Gui) OnUIThreadAndWait(f func()) error {
 	return g.onUIThreadAndWait(f, false)
 }
 
 // Like OnUIThreadAndWait, but the enqueued work belongs to a background routine,
 // so it doesn't count towards the program being busy (see UpdateBackground).
-func (g *Gui) OnUIThreadAndWaitBackground(f func() error) error {
+func (g *Gui) OnUIThreadAndWaitBackground(f func()) error {
 	return g.onUIThreadAndWait(f, true)
 }
 
-func (g *Gui) onUIThreadAndWait(f func() error, background bool) error {
+func (g *Gui) onUIThreadAndWait(f func(), background bool) error {
 	enqueue := g.Update
 	if background {
 		enqueue = g.UpdateBackground
 	}
 
-	result := make(chan error, 1)
+	ran := make(chan struct{})
 	enqueue(func(*Gui) error {
-		result <- f()
+		f()
+		close(ran)
 		return nil
 	})
-	return <-result
+
+	select {
+	case <-ran:
+		return nil
+	case <-g.loopExited:
+		// The queue we just enqueued onto is no longer being served, so waiting
+		// on `ran` here would mean waiting for the rest of the process's life.
+		return ErrLoopExited
+	}
 }
 
 // Calls a function in a goroutine. Handles panics gracefully and tracks

pkg/gocui/ui_thread_test.go

@@ -0,0 +1,43 @@
+package gocui
+
+import (
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// errStillWaiting stands in for the result of a wait that hasn't produced one.
+var errStillWaiting = errors.New("still waiting")
+
+// resultOrTimeout reports what a wait returned, or errStillWaiting if it hasn't
+// returned by the time we give up on it.
+func resultOrTimeout(result chan error) error {
+	select {
+	case err := <-result:
+		return err
+	case <-time.After(time.Second):
+		return errStillWaiting
+	}
+}
+
+// A worker waiting for the UI thread must not be left parked there once the
+// main loop has stopped: nothing will ever run its callback, and the shutdown
+// that follows blocks until such workers have finished (see
+// tasks.ViewBufferManager.Close).
+func TestOnUIThreadAndWaitGivesUpWhenTheLoopExits(t *testing.T) {
+	g := newTestGui(t)
+
+	// Closing this is what MainLoop returning does. From here on nothing
+	// dequeues user events, so the callback below is never going to run.
+	close(g.loopExited)
+
+	result := make(chan error, 1)
+	go func() {
+		result <- g.OnUIThreadAndWait(func() {})
+	}()
+
+	err := resultOrTimeout(result)
+	assert.ErrorIs(t, err, ErrLoopExited)
+}

pkg/gui/background.go

@@ -119,13 +119,12 @@ func (self *BackgroundRoutineMgr) startBackgroundFetch() {
 		var appStatusHelper *helpers.AppStatusHelper
 		var branchesHelper *helpers.BranchesHelper
 		var fetchGeneration int
-		if err := self.gui.g.OnUIThreadAndWaitBackground(func() error {
+		if err := self.gui.g.OnUIThreadAndWaitBackground(func() {
 			git = self.gui.git
 			appStatusHelper = self.gui.helpers.AppStatus
 			branchesHelper = self.gui.helpers.BranchesHelper
 			fetchGeneration = self.gui.c.State().GetRepoGeneration()
 			self.gui.State.LastBackgroundFetchTime = time.Now()
-			return nil
 		}); err != nil {
 			return err
 		}
@@ -184,10 +183,9 @@ func (self *BackgroundRoutineMgr) checkForExternalChanges() {
 	// reading them from this background goroutine would race the reassignment.
 	var git *commands.GitCommand
 	var refreshHelper *helpers.RefreshHelper
-	if err := self.gui.g.OnUIThreadAndWaitBackground(func() error {
+	if err := self.gui.g.OnUIThreadAndWaitBackground(func() {
 		git = self.gui.git
 		refreshHelper = self.gui.helpers.Refresh
-		return nil
 	}); err != nil {
 		return
 	}

pkg/gui/controllers/helpers/merge_and_rebase_helper.go

@@ -188,9 +188,8 @@ func (self *MergeAndRebaseHelper) hasExecTodos(calledFromWorker bool) bool {
 	}
 
 	result := false
-	_ = self.c.GocuiGui().OnUIThreadAndWait(func() error {
+	_ = self.c.GocuiGui().OnUIThreadAndWait(func() {
 		result = check()
-		return nil
 	})
 	return result
 }

pkg/gui/controllers/helpers/refresh_helper.go

@@ -223,10 +223,12 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 		background:        options.Background || options.DontBlockRepoSwitch,
 		backgroundRoutine: options.Background,
 	}
-	self.captureOnUIThread(calledFromWorker, env.background, func() {
+	if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 		env.generation = self.c.State().GetRepoGeneration()
 		env.git = self.c.Git()
-	})
+	}) {
+		return
+	}
 	if options.BatchUIUpdates {
 		env.batch = &refreshBounceBatch{}
 	}
@@ -321,11 +323,13 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 		var capturedCommits capturedCommitState
 		var capturedReflog capturedReflogState
 		var capturedBranches capturedBranchState
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			capturedCommits = self.captureCommitsState()
 			capturedReflog = self.captureReflogState()
 			capturedBranches = self.captureBranchState()
-		})
+		}) {
+			return
+		}
 		refresh("commits and commit files", func() {
 			self.refreshCommitsAndCommitFiles(capturedCommits, options.CommitSelection, env)
 		})
@@ -355,35 +359,43 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 		// if we've asked specifically for rebase commits and not those other things
 		var rebaseHashPool *utils.StringPool
 		var rebaseCommits []*models.Commit
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			rebaseHashPool, rebaseCommits = self.captureRebaseCommitState()
-		})
+		}) {
+			return
+		}
 		refresh("rebase commits", func() { _ = self.refreshRebaseCommits(rebaseHashPool, rebaseCommits, env) })
 	}
 
 	if scopeSet.Includes(types.SUB_COMMITS) {
 		var capturedSubCommits capturedSubCommitState
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			capturedSubCommits = self.captureSubCommitState()
-		})
+		}) {
+			return
+		}
 		refresh("sub commits", func() { _ = self.refreshSubCommitsWithLimit(capturedSubCommits, env) })
 	}
 
 	// reason we're not doing this if the COMMITS type is included is that if the COMMITS type _is_ included we will refresh the commit files context anyway
 	if scopeSet.Includes(types.COMMIT_FILES) && !scopeSet.Includes(types.COMMITS) {
 		var capturedCommitFiles capturedCommitFilesState
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			capturedCommitFiles = self.captureCommitFilesState()
-		})
+		}) {
+			return
+		}
 		refresh("commit files", func() { _ = self.refreshCommitFilesContext(capturedCommitFiles, env) })
 	}
 
 	fileWg := sync.WaitGroup{}
 	if scopeSet.Includes(types.FILES) {
 		var capturedFiles capturedFilesState
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			capturedFiles = self.captureFilesState()
-		})
+		}) {
+			return
+		}
 		fileWg.Add(1)
 		refresh("files", func() {
 			_ = self.refreshFilesAndSubmodules(capturedFiles, env)
@@ -393,9 +405,11 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 
 	if scopeSet.Includes(types.STASH) {
 		var stashFilterPath string
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			stashFilterPath = self.c.Modes().Filtering.GetPath()
-		})
+		}) {
+			return
+		}
 		refresh("stash", func() { self.refreshStashEntries(stashFilterPath, env) })
 	}
 
@@ -408,9 +422,11 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 		// needs it to keep the remote-branches selection valid, and reading
 		// the Remotes context off the UI thread races its render.
 		var prevSelectedRemote *models.Remote
-		self.captureOnUIThread(calledFromWorker, env.background, func() {
+		if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 			prevSelectedRemote = self.c.Contexts().Remotes.GetSelected()
-		})
+		}) {
+			return
+		}
 		branchesAndRemotesWg.Add(1)
 		refresh("remotes", func() {
 			loadedRemotes, _ = self.refreshRemotes(prevSelectedRemote, env)
@@ -1248,21 +1264,20 @@ func (self *RefreshHelper) onUIThread(background bool, f func() error) {
 // waiting for a callback that only it can run), and capturing inline also
 // guarantees the snapshot reflects the state at the moment Refresh was called,
 // before the calling handler regains control and can mutate it.
-func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background bool, fn func()) {
+//
+// It returns false when fn didn't run because the app is shutting down, in
+// which case the caller must abandon the refresh rather than compute from a
+// snapshot that was never taken.
+func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background bool, fn func()) bool {
 	if !calledFromWorker {
 		fn()
-		return
+		return true
 	}
 
-	wrapped := func() error {
-		fn()
-		return nil
-	}
 	if background {
-		_ = self.c.GocuiGui().OnUIThreadAndWaitBackground(wrapped)
-	} else {
-		_ = self.c.GocuiGui().OnUIThreadAndWait(wrapped)
+		return self.c.GocuiGui().OnUIThreadAndWaitBackground(fn) == nil
 	}
+	return self.c.GocuiGui().OnUIThreadAndWait(fn) == nil
 }
 
 // capturedFilesState holds the files refresh's context/model inputs, gathered

pkg/gui/controllers/patch_building_controller.go

@@ -230,9 +230,8 @@ func (self *PatchBuildingController) discardSelectionFromCommit() error {
 		err := self.c.Git().Patch.DeletePatchesFromCommit(commits, commitIndex)
 		// Escape pops the patch-building context, so run it on the UI thread
 		// before the refresh below.
-		_ = self.c.GocuiGui().OnUIThreadAndWait(func() error {
+		_ = self.c.GocuiGui().OnUIThreadAndWait(func() {
 			self.c.Helpers().PatchBuilding.Escape()
-			return nil
 		})
 		return self.c.Helpers().MergeAndRebase.CheckMergeOrRebaseWithRefreshOptions(
 			err, types.RefreshOptions{})

pkg/gui/gui_driver.go

@@ -82,7 +82,7 @@ func (self *GuiDriver) WaitUntilIdle() {
 }
 
 func (self *GuiDriver) OnUIThreadAndWait(f func()) {
-	_ = self.gui.g.OnUIThreadAndWait(func() error { f(); return nil })
+	_ = self.gui.g.OnUIThreadAndWait(f)
 }
 
 func (self *GuiDriver) replayMouseEvent(x, y int, buttons tcell.ButtonMask) {

pkg/gui/tasks_adapter.go

@@ -80,9 +80,8 @@ func (gui *Gui) newStringTaskWithoutScroll(view *gocui.View, str string) error {
 	manager := gui.getManager(view)
 
 	f := func(tasks.TaskOpts) error {
-		return gui.g.OnUIThreadAndWaitBackground(func() error {
+		return gui.g.OnUIThreadAndWaitBackground(func() {
 			gui.c.SetViewContent(view, str)
-			return nil
 		})
 	}
 
@@ -97,10 +96,9 @@ func (gui *Gui) newStringTaskWithScroll(view *gocui.View, str string, originX in
 	manager := gui.getManager(view)
 
 	f := func(tasks.TaskOpts) error {
-		return gui.g.OnUIThreadAndWaitBackground(func() error {
+		return gui.g.OnUIThreadAndWaitBackground(func() {
 			gui.c.SetViewContent(view, str)
 			view.SetOrigin(originX, originY)
-			return nil
 		})
 	}
 
@@ -115,10 +113,9 @@ func (gui *Gui) newStringTaskWithKey(view *gocui.View, str string, key string) e
 	manager := gui.getManager(view)
 
 	f := func(tasks.TaskOpts) error {
-		return gui.g.OnUIThreadAndWaitBackground(func() error {
+		return gui.g.OnUIThreadAndWaitBackground(func() {
 			gui.c.ResetViewOrigin(view)
 			gui.c.SetViewContent(view, str)
-			return nil
 		})
 	}
 

pkg/tasks/tasks.go

@@ -87,7 +87,7 @@ type ViewBufferManager struct {
 	// of the view happen through this, so that the view is only ever touched on
 	// the UI thread (where it is also laid out and drawn), never on the task's
 	// own goroutine.
-	onUIThread func(f func() error) error
+	onUIThread func(f func()) error
 
 	// if the user flicks through a heap of items, with each one
 	// spawning a process to render something to the main view,
@@ -126,7 +126,7 @@ func NewViewBufferManager(
 	onEndOfInput func(),
 	onNewKey func(),
 	newGocuiTask func() gocui.Task,
-	onUIThread func(f func() error) error,
+	onUIThread func(f func()) error,
 ) *ViewBufferManager {
 	return &ViewBufferManager{
 		Log:          log,
@@ -358,10 +358,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 							// onEndOfInput reads the view's dimensions (to decide
 							// whether to scroll) and sets the origin, both of which
 							// are UI-thread-only, so run it there.
-							_ = self.onUIThread(func() error {
-								self.onEndOfInput()
-								return nil
-							})
+							_ = self.onUIThread(self.onEndOfInput)
 							callThen()
 							break outer
 						}
@@ -502,10 +499,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error
 			// must happen after releasing taskIDMutex: it blocks until the UI
 			// thread runs it, and a NewTask call on the UI thread takes
 			// taskIDMutex, so holding it here would deadlock.
-			_ = self.onUIThread(func() error {
-				self.onNewKey()
-				return nil
-			})
+			_ = self.onUIThread(self.onNewKey)
 		}
 
 		self.waitingMutex.Lock()

pkg/tasks/tasks_test.go

@@ -40,7 +40,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) {
 		onNewKey,
 		newTask,
 		// no UI thread in the test; run the view mutations inline
-		func(f func() error) error { return f() },
+		func(f func()) error { f(); return nil },
 	)
 
 	stop := make(chan struct{})
@@ -107,7 +107,7 @@ func TestNewCmdTask(t *testing.T) {
 		onNewKey,
 		newTask,
 		// no UI thread in the test; run the view mutations inline
-		func(f func() error) error { return f() },
+		func(f func()) error { f(); return nil },
 	)
 
 	stop := make(chan struct{})
@@ -242,7 +242,7 @@ func TestNewCmdTaskRefresh(t *testing.T) {
 			func() {},
 			newTask,
 			// no UI thread in the test; run the view mutations inline
-			func(f func() error) error { return f() },
+			func(f func()) error { f(); return nil },
 		)
 
 		stop := make(chan struct{})