diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 20beb2af7..2785f07d4 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -1470,6 +1470,11 @@ func (g *Gui) drawListFooter(v *View, fgColor, bgColor Attribute) error { // flush updates the gui, re-drawing frames and buffers. func (g *Gui) flush() error { + // The screen must not be touched while suspended (see Suspend). + if g.isSuspended() { + return nil + } + // pretty sure we don't need this, but keeping it here in case we get weird visual artifacts // g.clear(g.FgColor, g.BgColor) @@ -1502,6 +1507,11 @@ func (g *Gui) flush() error { // actually-changed cells are emitted to the terminal. // Will also redraw any views that overlap tainted views func (g *Gui) flushContentOnly(views []*View) error { + // The screen must not be touched while suspended (see Suspend). + if g.isSuspended() { + return nil + } + for _, v := range viewsToRedrawContentOnly(views) { if err := g.draw(v); err != nil { return err @@ -1555,10 +1565,6 @@ func (g *Gui) ForceFlushViewsContentOnly(views []*View) error { // draw manages the cursor and calls the draw function of a view. func (g *Gui) draw(v *View) error { - if g.suspended { - return nil - } - if !v.Visible || v.y1 < v.y0 || v.x1 < v.x0 { return nil } @@ -1930,6 +1936,14 @@ func (g *Gui) onFocus(ev *GocuiEvent) error { return nil } +// While g.suspended is true, nothing must be drawn to the screen: tcell +// releases the screen's cell buffer when disengaging, and drawing to a +// disengaged screen spins forever inside tcell while holding the screen lock, +// which then blocks Resume (and with it all further input) forever. For the +// flag to guarantee that, it must only ever be false while the screen is +// engaged: Suspend sets it before disengaging, and Resume clears it only +// after re-engaging. + func (g *Gui) Suspend() error { g.suspendedMutex.Lock() defer g.suspendedMutex.Unlock() @@ -1940,7 +1954,12 @@ func (g *Gui) Suspend() error { g.suspended = true - return g.screen.Suspend() + if err := g.screen.Suspend(); err != nil { + g.suspended = false + return err + } + + return nil } func (g *Gui) Resume() error { @@ -1951,9 +1970,20 @@ func (g *Gui) Resume() error { return errors.New("Cannot resume because we are not suspended") } + if err := g.screen.Resume(); err != nil { + return err + } + g.suspended = false - return g.screen.Resume() + return nil +} + +func (g *Gui) isSuspended() bool { + g.suspendedMutex.Lock() + defer g.suspendedMutex.Unlock() + + return g.suspended } // matchView returns if the keybinding matches the current view (and the view's context) diff --git a/pkg/gocui/suspend_test.go b/pkg/gocui/suspend_test.go index 25a15c921..d5ea5a132 100644 --- a/pkg/gocui/suspend_test.go +++ b/pkg/gocui/suspend_test.go @@ -7,6 +7,51 @@ import ( "github.com/stretchr/testify/assert" ) +// A flush while suspended must return without touching the screen: tcell +// releases the screen's cell buffer when disengaging, and drawing to a +// disengaged screen spins forever inside tcell while holding the screen lock, +// blocking the resume triggered by fg (#5309). The flush runs in a goroutine +// so that a regression fails the test instead of hanging the suite. +func TestFlushIsNoOpWhileSuspended(t *testing.T) { + tests := []struct { + name string + flush func(g *Gui) error + }{ + {"flush", func(g *Gui) error { return g.flush() }}, + {"flushContentOnly", func(g *Gui) error { return g.flushContentOnly(g.views) }}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Deliberately not newTestGui: its cleanup closes the screen, + // which would deadlock on the screen lock if a regression makes + // the flush below spin. + g, err := NewGui(NewGuiOpts{ + OutputMode: OutputNormal, + Headless: true, + Width: 80, + Height: 24, + }) + assert.NoError(t, err) + + assert.NoError(t, g.Suspend()) + + flushReturned := make(chan error, 1) + go func() { flushReturned <- tc.flush(g) }() + + select { + case err := <-flushReturned: + assert.NoError(t, err) + case <-time.After(time.Second): + t.Fatal("flush touched the suspended screen and got stuck") + } + + assert.NoError(t, g.Resume()) + g.Close() + }) + } +} + func TestResumeSchedulesRedraw(t *testing.T) { g := newTestGui(t)