diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index be52c8584b6..a9b68dc5c8d 100644 --- a/pkg/gocui/gui.go +++ b/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 diff --git a/pkg/gocui/ui_thread_test.go b/pkg/gocui/ui_thread_test.go new file mode 100644 index 00000000000..d76bfaf9c8c --- /dev/null +++ b/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) +} diff --git a/pkg/gui/background.go b/pkg/gui/background.go index 180b2d44545..6a4c529a141 100644 --- a/pkg/gui/background.go +++ b/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 } diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index 7c7ab3e9a49..8f27efa08d4 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/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 } diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 53505ba6c8c..6672fab2656 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/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 diff --git a/pkg/gui/controllers/patch_building_controller.go b/pkg/gui/controllers/patch_building_controller.go index f3e26e3031f..e1405463a2d 100644 --- a/pkg/gui/controllers/patch_building_controller.go +++ b/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{}) diff --git a/pkg/gui/gui_driver.go b/pkg/gui/gui_driver.go index 25bfcf2d51e..673d9d726c7 100644 --- a/pkg/gui/gui_driver.go +++ b/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) { diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 3dce93874aa..ccc0b308cbb 100644 --- a/pkg/gui/tasks_adapter.go +++ b/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 }) } diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 3768e0c1936..df7791aafc6 100644 --- a/pkg/tasks/tasks.go +++ b/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() diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 2cea139e8b3..dec476ba4ff 100644 --- a/pkg/tasks/tasks_test.go +++ b/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{})