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..c54f5b9 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,191 @@ 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) + second, polled := newHoldingServer(t, session) + + app := &App{ui: NewUI(), nick: testNick} + runUI(t, app.ui) + + app.cmdConnect(first.URL) + app.cmdConnect(second.URL) + waitPoll(t, polled, "the second server") + + // 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() +} + +// TestSecondConnectIgnoresHeldPoll holds a poll of the earlier +// connection at its server while a second /connect runs, then +// answers it with a cursor and a message. It fails if that answer +// moves the cursor or shows the message. +func TestSecondConnectIgnoresHeldPoll(t *testing.T) { + t.Parallel() + + const session = `{"id": 1, "nick": "erin"}` + + const answer = `{"messages": [{"command": "PRIVMSG", "from": "bob", + "to": "#chan", "body": ["hello"]}], "last_id": 42}` + + // The first server sends on polled when the poll arrives, and + // answers it once release is closed. + polled := make(chan struct{}) + release := make(chan struct{}) + first := httptest.NewServer(http.HandlerFunc( + func(writer http.ResponseWriter, request *http.Request) { + select { + case polled <- struct{}{}: + case <-request.Context().Done(): + return + } + + select { + case <-release: + _, _ = io.WriteString(writer, answer) + case <-request.Context().Done(): + } + }, + )) + t.Cleanup(first.Close) + t.Cleanup(first.CloseClientConnections) + + // The second server answers every request at once with the + // session. A poll answered with it has no cursor and no messages, + // so the second poll loop never moves the cursor. + second := newServer(t, session) + + // The test runs the earlier connection's poll loop itself, so + // that it can wait for the loop to end. + stop := make(chan struct{}) + app := &App{ + ui: NewUI(), + client: api.NewClient(first.URL), + nick: testNick, + connected: true, + stopPoll: stop, + } + runUI(t, app.ui) + + done := make(chan struct{}) + + go func() { + app.pollLoop(stop) + close(done) + }() + + select { + case <-polled: + case <-time.After(eventLoopTimeout): + t.Fatalf("the first server got no poll within %v", eventLoopTimeout) + } + + app.cmdConnect(second.URL) + close(release) + + select { + case <-done: + case <-time.After(eventLoopTimeout): + t.Fatalf("the earlier poll loop did not end within %v", eventLoopTimeout) + } + + app.mu.Lock() + cursor := app.lastQID + app.mu.Unlock() + + if cursor != 0 { + t.Errorf("the earlier connection's answer moved the cursor to %d", cursor) + } + + lines, _ := bufferState(app.ui, testChannel) + if lines != 0 { + t.Errorf("the earlier connection's message was shown in %s", testChannel) + } + + // Stop the second poll loop as /quit does, without stopping the UI. + app.mu.Lock() + close(app.stopPoll) + app.mu.Unlock() +} + +// TestSecondConnectAfterFailedPoll holds a poll of the earlier +// connection at its server while a second /connect runs, then +// fails it. A poll loop waits pollRetry after a failed poll, then +// polls again. The test fails if the earlier poll loop polls the +// second server. +func TestSecondConnectAfterFailedPoll(t *testing.T) { + t.Parallel() + + const session = `{"id": 1, "nick": "erin"}` + + first, firstPolled := newHoldingServer(t, session) + + // The second server holds the one poll of the second + // connection's poll loop, so any other poll comes from the + // earlier loop. + second, secondPolled := newHoldingServer(t, session) + + // The test runs the earlier connection's poll loop itself, so + // that it can wait for the loop to end. + stop := make(chan struct{}) + app := &App{ + ui: NewUI(), + client: api.NewClient(first.URL), + nick: testNick, + connected: true, + stopPoll: stop, + } + runUI(t, app.ui) + + done := make(chan struct{}) + + go func() { + app.pollLoop(stop) + close(done) + }() + + waitPoll(t, firstPolled, "the first server") + app.cmdConnect(second.URL) + waitPoll(t, secondPolled, "the second server") + + // Closing the held poll's connection makes that poll fail. + first.CloseClientConnections() + + select { + case <-done: + case <-secondPolled: + t.Fatal("the earlier poll loop polled the second server") + case <-time.After(eventLoopTimeout): + t.Fatalf("the earlier poll loop did not end within %v", eventLoopTimeout) + } + + // Stop the second poll loop as /quit does, without stopping the UI. + 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 @@ -706,6 +892,53 @@ func newServer(t *testing.T, body string) *httptest.Server { return server } +// newHoldingServer starts a server that sends on the returned +// channel for each poll and holds the poll until its connection +// closes. It answers any other request with body. +func newHoldingServer( + t *testing.T, body string, +) (*httptest.Server, <-chan struct{}) { + t.Helper() + + polled := make(chan struct{}) + server := 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, body) + + 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(server.Close) + t.Cleanup(server.CloseClientConnections) + + return server, polled +} + +// waitPoll fails the test unless server gets a poll, sent on +// polled, within eventLoopTimeout. +func waitPoll(t *testing.T, polled <-chan struct{}, server string) { + t.Helper() + + select { + case <-polled: + case <-time.After(eventLoopTimeout): + t.Fatalf("%s got no poll within %v", server, eventLoopTimeout) + } +} + // runUI runs ui on a simulated screen until the test ends. func runUI(t *testing.T, ui *UI) { t.Helper()