From f9b790a1f916883a82b323e30917f3feb589b14c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 12 Aug 2026 11:04:53 +0200 Subject: [PATCH] Let OnUIThreadAndWait's error be about the wait, not about f Every caller passes an f that unconditionally returns nil, so f's error return has never carried anything: the value is dead weight, and it occupies the one channel the wait itself needs to report that it couldn't run f at all. Drop it, so that the error the wait returns can only ever mean that. Work that can fail hands its error back through a captured variable, the way the background fetch already hands back four values, which keeps the two outcomes distinguishable at a call site that has both. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 22 ++++++++++++------- pkg/gui/background.go | 6 ++--- .../helpers/merge_and_rebase_helper.go | 3 +-- pkg/gui/controllers/helpers/refresh_helper.go | 8 ++----- .../controllers/patch_building_controller.go | 3 +-- pkg/gui/gui_driver.go | 2 +- pkg/gui/tasks_adapter.go | 9 +++----- pkg/tasks/tasks.go | 14 ++++-------- pkg/tasks/tasks_test.go | 6 ++--- 9 files changed, 31 insertions(+), 42 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index be52c8584..43841b826 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -893,36 +893,42 @@ 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. 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 + <-ran + return nil } // Calls a function in a goroutine. Handles panics gracefully and tracks diff --git a/pkg/gui/background.go b/pkg/gui/background.go index 180b2d445..6a4c529a1 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 7c7ab3e9a..8f27efa08 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 53505ba6c..ce0cfe25e 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -1254,14 +1254,10 @@ func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background b return } - wrapped := func() error { - fn() - return nil - } if background { - _ = self.c.GocuiGui().OnUIThreadAndWaitBackground(wrapped) + _ = self.c.GocuiGui().OnUIThreadAndWaitBackground(fn) } else { - _ = self.c.GocuiGui().OnUIThreadAndWait(wrapped) + _ = self.c.GocuiGui().OnUIThreadAndWait(fn) } } diff --git a/pkg/gui/controllers/patch_building_controller.go b/pkg/gui/controllers/patch_building_controller.go index f3e26e303..e1405463a 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 25bfcf2d5..673d9d726 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 3dce93874..ccc0b308c 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 3768e0c19..df7791aaf 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 2cea139e8..dec476ba4 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{})