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