From 8710f98b1faa06da681d38f19aa084725892ccc2 Mon Sep 17 00:00:00 2001 From: Daisuke Maki Date: Mon, 16 Feb 2026 21:35:56 +0900 Subject: [PATCH] Remove the possibility of Resume() deadlocking --- action.go | 2 +- interface.go | 2 +- peco_test.go | 2 +- screen.go | 17 +++++-- screen_inline.go | 2 +- screen_test.go | 115 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 132 insertions(+), 8 deletions(-) create mode 100644 screen_test.go diff --git a/action.go b/action.go index 60fbda7..755c962 100644 --- a/action.go +++ b/action.go @@ -369,7 +369,7 @@ func doFinish(ctx context.Context, state *Peco, _ Event) { state.screen.Suspend() err = cmd.Run() - state.screen.Resume() + state.screen.Resume(ctx) state.Hub().SendDraw(ctx, &DrawOptions{DisableCache: true}) if err != nil { // bail out, or otherwise the user cannot know what happened diff --git a/interface.go b/interface.go index e106dc8..23c8056 100644 --- a/interface.go +++ b/interface.go @@ -180,7 +180,7 @@ type Screen interface { Flush() error PollEvent(context.Context, *Config) chan Event Print(PrintArgs) int - Resume() + Resume(context.Context) SetCell(int, int, rune, Attribute, Attribute) SetCursor(int, int) Size() (int, int) diff --git a/peco_test.go b/peco_test.go index e05faa3..0b3eb52 100644 --- a/peco_test.go +++ b/peco_test.go @@ -244,7 +244,7 @@ func (s *SimScreen) Size() (int, int) { return s.screen.Size() } -func (s *SimScreen) Resume() {} +func (s *SimScreen) Resume(_ context.Context) {} func (s *SimScreen) Suspend() {} // Sync records a "Sync" event via the interceptor. This satisfies the diff --git a/screen.go b/screen.go index a16ca1d..3d7ac32 100644 --- a/screen.go +++ b/screen.go @@ -301,17 +301,26 @@ func (t *Termbox) Suspend() { } } -func (t *Termbox) Resume() { +func (t *Termbox) Resume(ctx context.Context) { // Resume must be a block operation, because we can't safely proceed // without actually knowing that the screen has been re-initialized. - // So we send a channel where we expect a reply back, and wait for that + // So we send a channel where we expect a reply back, and wait for that. + // + // Both selects are guarded by ctx.Done() to avoid deadlock: if the + // polling goroutine is not yet waiting on resumeCh, a non-blocking + // send would silently drop the message and the subsequent receive + // would block forever. ch := make(chan struct{}) select { case t.resumeCh <- ch: - default: + case <-ctx.Done(): + return } - <-ch + select { + case <-ch: + case <-ctx.Done(): + } } // SetCell writes to the terminal diff --git a/screen_inline.go b/screen_inline.go index 464cfb2..f516fa2 100644 --- a/screen_inline.go +++ b/screen_inline.go @@ -217,4 +217,4 @@ func (s *InlineScreen) SendEvent(_ Event) {} func (s *InlineScreen) Suspend() {} // Resume is a no-op for inline mode. -func (s *InlineScreen) Resume() {} +func (s *InlineScreen) Resume(_ context.Context) {} diff --git a/screen_test.go b/screen_test.go new file mode 100644 index 0000000..e404f74 --- /dev/null +++ b/screen_test.go @@ -0,0 +1,115 @@ +package peco + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestTermboxResumeNoDeadlock(t *testing.T) { + tb := NewTermbox() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + // Simulate the polling goroutine: receive from resumeCh after a short delay, + // then close the reply channel (as PollEvent does after re-init). + go func() { + time.Sleep(50 * time.Millisecond) + replyCh := <-tb.resumeCh + close(replyCh) + }() + + done := make(chan struct{}) + go func() { + tb.Resume(ctx) + close(done) + }() + + select { + case <-done: + // Resume completed without deadlock. + case <-time.After(2 * time.Second): + t.Fatal("Resume() deadlocked") + } +} + +func TestTermboxResumeDoesNotDropSend(t *testing.T) { + tb := NewTermbox() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + received := make(chan struct{}) + go func() { + replyCh := <-tb.resumeCh + close(received) + close(replyCh) + }() + + tb.Resume(ctx) + + select { + case <-received: + // The receiver goroutine got the message. + default: + t.Fatal("receiver did not get the resume message") + } +} + +func TestTermboxResumeContextCancelled(t *testing.T) { + tb := NewTermbox() + + ctx, cancel := context.WithCancel(context.Background()) + + // Cancel immediately so Resume cannot deliver on resumeCh. + cancel() + + done := make(chan struct{}) + go func() { + tb.Resume(ctx) + close(done) + }() + + select { + case <-done: + // Resume returned promptly after context cancellation. + case <-time.After(2 * time.Second): + t.Fatal("Resume() did not unblock after context cancellation") + } +} + +func TestTermboxResumeContextCancelledWhileWaitingForReply(t *testing.T) { + tb := NewTermbox() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Accept the resume request but never close the reply channel. + // This tests that the second select also respects ctx.Done(). + go func() { + <-tb.resumeCh // receive but don't close replyCh + }() + + done := make(chan struct{}) + go func() { + tb.Resume(ctx) + close(done) + }() + + // Give Resume time to pass the first select and block on the second. + time.Sleep(50 * time.Millisecond) + cancel() + + select { + case <-done: + // Resume returned after context cancellation during reply wait. + case <-time.After(2 * time.Second): + t.Fatal("Resume() did not unblock after context cancellation while waiting for reply") + } + + // Verify context was indeed cancelled. + require.Error(t, ctx.Err()) +}