From 2bb7a7834032982038b3f7738da59d228601e689 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 2 Nov 2026 02:26:58 +0000 Subject: [PATCH] Stop the earlier poll loop on a second /connect (closes #228) A second /connect started a new poll loop without stopping the first. The first loop then polled with the new client too, so each message could be shown twice. It also read stopPoll without a.mu while cmdConnect wrote it. cmdConnect now closes the earlier loop's channel in the same locked step that replaces the client and the cursor, and hands the new channel to the new loop. The loop checks its channel under a.mu before each poll and again before it moves the cursor, so a stopped loop never polls with the new client or moves the new cursor. Model: opus-5-5 --- internal/cli/app.go | 47 +++++++++++++++----- internal/cli/app_internal_test.go | 72 ++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 12 deletions(-) diff --git a/internal/cli/app.go b/internal/cli/app.go index 3e0bb80..207048b 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -199,11 +199,21 @@ func (a *App) cmdConnect(serverURL string) { return } + // The poll loop of an earlier connection is stopped in the same + // step that replaces the client, so it never polls with the new + // one. a.mu.Lock() a.client = client a.nick = resp.Nick a.connected = true a.lastQID = 0 + + if a.stopPoll != nil { + close(a.stopPoll) + } + + stop := make(chan struct{}) + a.stopPoll = stop a.mu.Unlock() a.ui.AddStatus(fmt.Sprintf( @@ -212,9 +222,7 @@ func (a *App) cmdConnect(serverURL string) { )) a.ui.SetStatus(resp.Nick, "", "connected") - a.stopPoll = make(chan struct{}) - - go a.pollLoop() + go a.pollLoop(stop) } func (a *App) cmdNick(nick string) { @@ -702,16 +710,23 @@ func (a *App) cmdHelp() { } } -// pollLoop long-polls for messages in the background. -func (a *App) pollLoop() { +// pollLoop long-polls for messages in the background until stop is +// closed. A later /connect closes stop under a.mu as it replaces the +// client and the cursor, so the loop checks stop under a.mu too. A +// stopped loop then neither polls with the new client nor moves the +// new cursor. +func (a *App) pollLoop(stop <-chan struct{}) { for { + a.mu.Lock() + select { - case <-a.stopPoll: + case <-stop: + a.mu.Unlock() + return default: } - a.mu.Lock() client := a.client lastQID := a.lastQID a.mu.Unlock() @@ -729,12 +744,24 @@ func (a *App) pollLoop() { continue } - if result.LastID > 0 { - a.mu.Lock() - a.lastQID = result.LastID + // A poll sent before stop was closed answers for the earlier + // connection. + a.mu.Lock() + + select { + case <-stop: a.mu.Unlock() + + return + default: } + if result.LastID > 0 { + a.lastQID = result.LastID + } + + a.mu.Unlock() + for i := range result.Messages { a.handleServerMessage(&result.Messages[i]) } diff --git a/internal/cli/app_internal_test.go b/internal/cli/app_internal_test.go index 9d097fa..2d88c75 100644 --- a/internal/cli/app_internal_test.go +++ b/internal/cli/app_internal_test.go @@ -28,8 +28,9 @@ const ( // it fails. const eventLoopTimeout = 5 * time.Second -// holdTime is how long TestLinesKeepOrder holds the first line, -// which gives the second line time to start too early. +// holdTime is how long a test waits to see that something does not +// happen: the second line handled while the first is held, or a +// poll from a poll loop that was stopped. const holdTime = 200 * time.Millisecond // welcomeLines is the number of lines run adds to the status @@ -422,6 +423,73 @@ func TestLinesKeepOrder(t *testing.T) { waitHandled(t, handled, secondLine) } +// TestSecondConnectStopsFirstPollLoop connects to one server, then +// to a second that holds every poll. Only the second connection's +// poll loop may poll the second server. +func TestSecondConnectStopsFirstPollLoop(t *testing.T) { + t.Parallel() + + const session = `{"id": 1, "nick": "erin"}` + + // The first server answers every request at once, so the first + // poll loop polls it until the second /connect. + first := newServer(t, session) + + // The second server sends on polled for each poll and holds the + // poll until its connection closes. It answers anything else + // with the session. + polled := make(chan struct{}) + second := httptest.NewServer(http.HandlerFunc( + func(writer http.ResponseWriter, request *http.Request) { + if request.Method != http.MethodGet || + request.URL.Path != "/api/v1/messages" { + _, _ = io.WriteString(writer, session) + + return + } + + select { + case polled <- struct{}{}: + case <-request.Context().Done(): + } + + <-request.Context().Done() + }, + )) + // Close waits for the held polls, which end only once their + // connections close. Cleanups run last first, so the connections + // close before Close runs. + t.Cleanup(second.Close) + t.Cleanup(second.CloseClientConnections) + + app := &App{ui: NewUI(), nick: testNick} + runUI(t, app.ui) + + app.cmdConnect(first.URL) + app.cmdConnect(second.URL) + + select { + case <-polled: + case <-time.After(eventLoopTimeout): + t.Fatalf("the second server got no poll within %v", eventLoopTimeout) + } + + // The second connection's poll loop waits on its held poll, so + // another poll comes from the first loop. + select { + case <-polled: + t.Fatal("the first poll loop polled again after the second /connect") + case <-time.After(holdTime): + } + + // Stop the second poll loop as /quit does. /quit itself also + // stops the UI, and runUI's cleanup stopping it again is a data + // race inside tview. + app.mu.Lock() + close(app.stopPoll) + app.mu.Unlock() +} + // TestStartUp runs the client's start-up path on a simulation // screen. It fails unless the event loop starts and the status // window gets two lines: first the one that holds "Welcome to