diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 558b9d619..ee1995911 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -238,7 +238,12 @@ func NewGui(opts NewGuiOpts) (*Gui, error) { g.stop = make(chan struct{}) g.gEvents = make(chan GocuiEvent, 20) - g.userEvents = make(chan userEvent, 20) + // Update does a non-blocking send and panics on a full channel rather than + // blocking (which would deadlock the UI goroutine against itself) or + // silently reordering. The buffer is sized well above the peak occupancy we + // see in practice, so the panic stays unreachable in normal use; if it ever + // fires, that's a real anomaly to investigate, not a cue to grow the buffer. + g.userEvents = make(chan userEvent, 256) g.taskManager = newTaskManager() if opts.PlayRecording { @@ -613,28 +618,23 @@ type userEvent struct { contentOnly bool } -// Update executes the passed function. This method can be called safely from a -// goroutine in order to update the GUI. It is important to note that the -// passed function won't be executed immediately, instead it will be added to -// the user events queue. Given that Update spawns a goroutine, the order in -// which the user events will be handled is not guaranteed. +// Update enqueues f on the user-events channel for the UI loop to run on its +// next iteration. Multiple Update calls from the same goroutine arrive in +// source order via the channel's FIFO. The send is non-blocking — if the +// channel is full we panic rather than block or silently reorder, since a +// blocked send from the UI goroutine would deadlock against itself and +// silently switching to inline execution would break the ordering guarantee +// callers rely on. The buffer is sized generously enough that this should +// never fire in practice; if it does, that's a signal to investigate, not +// to grow the buffer reflexively. func (g *Gui) Update(f func(*Gui) error) { task := g.NewTask() - go g.updateAsyncAux(f, task) -} - -// UpdateAsync is a version of Update that does not spawn a go routine, it can -// be a bit more efficient in cases where Update is called many times like when -// tailing a file. In general you should use Update() -func (g *Gui) UpdateAsync(f func(*Gui) error) { - task := g.NewTask() - - g.updateAsyncAux(f, task) -} - -func (g *Gui) updateAsyncAux(f func(*Gui) error, task Task) { - g.userEvents <- userEvent{f: f, task: task} + select { + case g.userEvents <- userEvent{f: f, task: task}: + default: + panic("gocui: userEvents channel full; refusing to block or reorder") + } } // Like Update, but signals that the callback only modifies content. diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index 82f4fcac0..03b7469d2 100644 --- a/pkg/gui/main_panels.go +++ b/pkg/gui/main_panels.go @@ -117,6 +117,8 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { } } + gui.moveMainContextPairToTop(opts.Pair) + if opts.Main != nil { gui.RefreshMainView(opts.Main, opts.Pair.Main) } @@ -127,8 +129,6 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { opts.Pair.Secondary.GetView().Clear() } - gui.moveMainContextPairToTop(opts.Pair) - gui.splitMainPanel(opts.Secondary != nil) } diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index f534d01b4..7fadbb451 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -403,12 +403,29 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error }) } + // Assign the taskID synchronously so it reflects NewTask call order + // rather than the order in which the spawned goroutines happen to be + // scheduled. Otherwise two NewTask calls in quick succession can have + // their goroutines race, with the later-called task ending up with the + // lower taskID and losing the staleness check below. + self.taskIDMutex.Lock() + self.newTaskID++ + taskID := self.newTaskID + self.taskIDMutex.Unlock() + go utils.Safe(func() { defer completeGocuiTask() self.taskIDMutex.Lock() - self.newTaskID++ - taskID := self.newTaskID + + // Bail out before touching shared view state if a newer task has + // already been queued: if we ran onNewKey here we'd reset the view + // for a task that's about to exit, potentially wiping output the + // winning task has already written. + if taskID < self.newTaskID { + self.taskIDMutex.Unlock() + return + } if self.GetTaskKey() != key && self.onNewKey != nil { self.onNewKey() @@ -419,6 +436,8 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error self.waitingMutex.Lock() + // Re-check staleness after acquiring waitingMutex: a newer task + // may have arrived while we were blocked here. self.taskIDMutex.Lock() if taskID < self.newTaskID { self.waitingMutex.Unlock()