diff --git a/pkg/gui/main_view_render.go b/pkg/gui/main_view_render.go index 5c615208d..4b661bf85 100644 --- a/pkg/gui/main_view_render.go +++ b/pkg/gui/main_view_render.go @@ -56,11 +56,16 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er // it before anything else can touch the command's arguments. cmdStr := strings.Join(cmd.Args, " ") + manager := gui.getManager(view) + // The task takes its place among the view's tasks now, although it is only + // created after the layout (see the matching call in newCmdTask). + reservation := manager.ReserveTask() + // Mark the view as loading synchronously now, before the layout pass: the // actual task is created in afterLayout (below), which runs after layout, so // without this the next layout pass would clamp the scroll position to the // not-yet-loaded content. - gui.getManager(view).StartLoading() + manager.StartLoading() // Hold the scrollbar at its current height while the re-render loads, so the // thumb doesn't shrink and snap back when the first partial paint swaps in // (see the matching call in newCmdTask). @@ -68,6 +73,10 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er // Run the render after layout so that it gets the correct size gui.afterLayout(func() error { + if manager.IsSuperseded(reservation) { + return nil + } + // The layout may have changed the size of the view, so only now is the // width to render at known, and with it the renderer command. width := gui.renderWidth(view) @@ -113,7 +122,7 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er if rendersThroughAPipe() { run = gui.pipedRender } - return gui.newTaskForRender(spec, prefix, cmdStr, run) + return gui.newTaskForRender(reservation, spec, prefix, cmdStr, run) }) return nil @@ -144,16 +153,17 @@ type ( type runRender func(spec renderSpec) (startRender, onCloseRender) // newTaskForRender creates the task that reads the render's output into its -// view, running the command the given way. key names what is rendered, so that +// view, running the command the given way. The task takes the place that the +// reservation holds among the view's tasks. key names what is rendered, so that // a re-render of the same content can be told from a render of other content. -func (gui *Gui) newTaskForRender(spec renderSpec, prefix string, key string, run runRender) error { +func (gui *Gui) newTaskForRender(reservation tasks.TaskReservation, spec renderSpec, prefix string, key string, run runRender) error { setColumnsEnvVar(spec.cmd, spec.width) start, onClose := run(spec) manager := gui.getManager(spec.view) linesToRead := gui.linesToReadFromCmdTask(spec.view) - return manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, onClose), key) + return manager.NewReservedTask(reservation, manager.NewCmdTask(start, prefix, linesToRead, onClose), key) } // renderWithoutPtyEnvVar makes a render take the piped path on a platform that diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 644b642a9..4c37a18ee 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -18,10 +18,16 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error cmdStr, ).Debug("RunCommand") + manager := gui.getManager(view) + // The task is only created after the layout (see below), but it has to take + // its place among the view's tasks now. Otherwise a task asked for after this + // one, but created before the layout, would be replaced by it. + reservation := manager.ReserveTask() + // Mark the view as loading synchronously (before the task's goroutine runs // and before the next layout pass) so the layout doesn't clamp the scroll // position to the not-yet-loaded content. - gui.getManager(view).StartLoading() + manager.StartLoading() // Hold the scrollbar at the height the view has now (the previous render), // while it still shows that render: once the re-render swaps in its first // partial paint the displayed buffer is briefly short, and we don't want the @@ -34,8 +40,12 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // keeps the task goroutine from reading the view's live dimensions while it // streams output. gui.afterLayout(func() error { + if manager.IsSuperseded(reservation) { + return nil + } + spec := renderSpec{view: view, cmd: cmd, width: gui.renderWidth(view)} - return gui.newTaskForRender(spec, prefix, cmdStr, gui.plainRender) + return gui.newTaskForRender(reservation, spec, prefix, cmdStr, gui.plainRender) }) return nil diff --git a/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go b/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go index df33d3b72..962185f6e 100644 --- a/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go +++ b/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go @@ -34,9 +34,6 @@ var ShowTheTabSwitchedToLast = NewIntegrationTest(NewIntegrationTestArgs{ IsFocused() t.Views().Main(). - /* EXPECTED: Content(Contains("Path:")) - ACTUAL: */ - Content(Contains("diff --git a/file1 b/file1")) }, }) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index c1656b2de..46452ab1c 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -64,9 +64,10 @@ type ViewBufferManager struct { waitingMutex deadlock.Mutex // Guards newTaskID and taskKey, which identify the most recently requested - // task. Both are written on the goroutine NewTask spawns, and taskKey is - // read from the UI thread (GetTaskKey), so neither may be touched without - // holding this. + // task. newTaskID is written wherever a task is asked for (ReserveTask) and + // read on the goroutine NewReservedTask spawns. taskKey is written on that + // goroutine and read from the UI thread (GetTaskKey). So neither may be + // touched without holding this. taskIDMutex deadlock.Mutex Log *logrus.Entry newTaskID int @@ -817,7 +818,43 @@ type TaskOpts struct { InitialContentLoaded func() } +// A TaskReservation holds a task's place in the order of the view's tasks, from +// when the task is asked for until it is created. See ReserveTask. +type TaskReservation struct { + taskID int +} + +// ReserveTask gives a task its place in the order of the view's tasks, for a task +// that is only created later, with NewReservedTask. The view shows the task that +// was asked for last. If another task is asked for after the reservation, that +// task replaces the reserved one, even when it is created first. +func (self *ViewBufferManager) ReserveTask() TaskReservation { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + self.newTaskID++ + return TaskReservation{taskID: self.newTaskID} +} + +// IsSuperseded reports whether another task has been asked for since the +// reservation was made. The reserved task would then stop as soon as it was +// created, so there is no point in creating it. +func (self *ViewBufferManager) IsSuperseded(reservation TaskReservation) bool { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + return reservation.taskID < self.newTaskID +} + +// NewTask creates a task that takes its place in the order of the view's tasks +// right away. func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error { + return self.NewReservedTask(self.ReserveTask(), f, key) +} + +// NewReservedTask creates the task that the reservation was made for. It doesn't +// run if another task has been asked for since the reservation was made. +func (self *ViewBufferManager) NewReservedTask(reservation TaskReservation, f func(TaskOpts) error, key string) error { gocuiTask := self.newGocuiTask() var completeTaskOnce sync.Once @@ -828,15 +865,7 @@ 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() + taskID := reservation.taskID go utils.Safe(func() { defer completeGocuiTask() diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 7383070e7..41139b9f1 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -813,3 +813,60 @@ func TestReadToEndHoldsItsTaskUntilThenReturns(t *testing.T) { assert.Equal(t, gocui.TaskStatusBusy, statusDuringThen) assert.Equal(t, gocui.TaskStatusDone, task.Status()) } + +// doneSignallingTask lets a test wait for the tasks of a view to finish, including +// those that never get to run. +type doneSignallingTask struct { + *gocui.FakeTask + done func() +} + +func (self *doneSignallingTask) Done() { + self.FakeTask.Done() + self.done() +} + +// A render whose task is only created after the layout takes its place among the +// view's tasks when it is asked for. A message asked for after it, before the +// layout, is created first, and the render's task mustn't replace it. +func TestReservedTaskDoesntReplaceATaskAskedForLater(t *testing.T) { + var tasksDone sync.WaitGroup + manager := NewViewBufferManager( + utils.NewDummyLog(), + bytes.NewBuffer(nil), + func() {}, + func() {}, + func() {}, + func() {}, + func() {}, + func() {}, + func() gocui.Task { + tasksDone.Add(1) + return &doneSignallingTask{FakeTask: gocui.NewFakeTask(), done: tasksDone.Done} + }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + var mutex sync.Mutex + var tasksRun []string + task := func(name string) func(TaskOpts) error { + return func(TaskOpts) error { + mutex.Lock() + defer mutex.Unlock() + tasksRun = append(tasksRun, name) + return nil + } + } + + reservation := manager.ReserveTask() + assert.False(t, manager.IsSuperseded(reservation)) + + _ = manager.NewTask(task("message"), "message") + assert.True(t, manager.IsSuperseded(reservation)) + + _ = manager.NewReservedTask(reservation, task("render"), "render") + tasksDone.Wait() + + assert.Equal(t, []string{"message"}, tasksRun) +}