Fix hang on quit when confirmOnQuit is true

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()