From d887a41ad2c386ca8b43fab9262553525e57af72 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 17 Jul 2026 13:58:32 +0200 Subject: [PATCH] Don't flush the screen while suspended When suspending with ctrl+z, the suspend keybinding handler disengages the screen and then sends SIGSTOP to the process group, so the UI thread freezes at the return from kill(2) with the handler's follow-up flush still pending. When fg continues the process, that pending flush races the SIGCONT handler's Resume. If the flush wins, Show() draws against the disengaged screen, whose cell buffer tcell has released to 0x0 while its width/height still hold the old size; drawCell() then reports width 0 for the out-of-range cell, the draw loop's 'x += width - 1' never advances, and the UI thread spins forever while holding the tcell screen lock. Resume in turn blocks forever on that lock, so the screen never re-engages and no input is ever read again: the hard stall of #5309, only recoverable by killing the process. Guard both flush paths with the suspended flag. For the flag to guarantee that the screen is engaged whenever it is false, Resume must clear it only after re-engaging (it used to clear it before); Suspend already sets it before disengaging. This also covers the pre-existing unsynchronized suspended check in draw(), which is subsumed by the guards and can go. The regression test cannot use the demonstrate-then-fix pattern: on unfixed code the flush goroutine spins holding the screen lock, which deadlocks any subsequent screen call including the test cleanup's Close(). --- pkg/gocui/gui.go | 42 ++++++++++++++++++++++++++++++------ pkg/gocui/suspend_test.go | 45 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 6 deletions(-) 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)