From cfc3736e8e2e2beade50ce5efc5c2eff66fced4c Mon Sep 17 00:00:00 2001 From: Daisuke Maki Date: Tue, 17 Feb 2026 07:38:56 +0900 Subject: [PATCH] log pollevent errors --- screen.go | 11 ++++++++++- screen_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/screen.go b/screen.go index 2b199de..78fba85 100644 --- a/screen.go +++ b/screen.go @@ -3,6 +3,9 @@ package peco import ( "context" "fmt" + "io" + "os" + "runtime/debug" "sync" "unicode/utf8" @@ -21,6 +24,7 @@ type TcellScreen struct { suspendCh chan struct{} doneCh chan struct{} // closed on permanent Close() to signal goroutines to exit closeOnce sync.Once // ensures doneCh is closed exactly once + errWriter io.Writer // destination for error output (defaults to os.Stderr) } // tcellKeyToKeyseq maps tcell navigation/function key constants to peco keyseq constants. @@ -177,6 +181,7 @@ func NewTcellScreen() *TcellScreen { suspendCh: make(chan struct{}), resumeCh: make(chan chan struct{}), doneCh: make(chan struct{}), + errWriter: os.Stderr, } } @@ -268,7 +273,11 @@ func (t *TcellScreen) PollEvent(ctx context.Context, cfg *Config) chan Event { }() go func() { - defer func() { recover() }() + defer func() { + if r := recover(); r != nil { + fmt.Fprintf(t.errWriter, "peco: panic in PollEvent goroutine: %v\n%s", r, debug.Stack()) + } + }() defer func() { close(evCh) }() for { diff --git a/screen_test.go b/screen_test.go index 4331b1d..72cea73 100644 --- a/screen_test.go +++ b/screen_test.go @@ -1,13 +1,60 @@ package peco import ( + "bytes" "context" "testing" "time" + "github.com/gdamore/tcell/v2" "github.com/stretchr/testify/require" ) +// panickingScreen wraps a tcell.Screen and panics on PollEvent. +// Used to test that TcellScreen's PollEvent goroutine logs panics +// instead of silently swallowing them. +type panickingScreen struct { + tcell.Screen +} + +func (s *panickingScreen) PollEvent() tcell.Event { + panic("test: deliberate panic in PollEvent") +} + +// TestTcellScreenPollEventLogsPanic verifies that when a panic occurs +// in the PollEvent goroutine, it is logged to errWriter rather than +// being silently swallowed (the bug described in CODE_REVIEW.md ยง3.2). +func TestTcellScreenPollEventLogsPanic(t *testing.T) { + var buf bytes.Buffer + ts := NewTcellScreen() + ts.errWriter = &buf + + // Set the screen to a wrapper that panics on PollEvent. + sim := tcell.NewSimulationScreen("") + sim.Init() + ts.screen = &panickingScreen{Screen: sim} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + evCh := ts.PollEvent(ctx, nil) + + // The goroutine should panic, log it, and close the channel. + select { + case _, ok := <-evCh: + require.False(t, ok, "expected channel to be closed after panic") + case <-time.After(2 * time.Second): + t.Fatal("PollEvent channel was not closed after panic") + } + + // Verify that the panic was logged (not silently swallowed). + output := buf.String() + require.Contains(t, output, "peco: panic in PollEvent goroutine") + require.Contains(t, output, "test: deliberate panic in PollEvent") + + ts.Close() +} + // TestTcellScreenSuspendHandlerExitsOnClose verifies that the suspend handler // goroutine (started by PollEvent) exits when Close() is called, even if // the context has not been cancelled. This is the goroutine leak described