Skip to content

Follow-ups from #730: a sleep used as synchronisation in read-waiting-in-window, start-helper comments stale after #747 #782

Description

@FumingPower3925

Follow-ups from the review of #730 (#705, part of #706, item 1 of #716). None of them blocks the merge: the maintainer capped reviews at two rounds on 2026-09-27, and these are minors.

  1. read-waiting-in-window uses a sleep as synchronisation (CodeRabbit, minor; middleware/websocket/engineread_latespill_test.go, the third case of TestChanReaderSpillPublishedAfterDrain, around lines 298-313). The window callback starts the async Read and then calls time.Sleep(50 * time.Millisecond) so that the Read reaches its wait decision before the chunk is published. On a loaded host the sleep can end before the Read starts. The chunk is then published first, and the case passes without exercising the in-window interleaving. The case's comment already says so: it can pass wrongly and cannot fail wrongly. Suggested fix: replace the sleep with waitLockWaiter(t, "(*chanReader).promoteSpill"), which TestChanReaderCloseKeepsChunksAppendedBeforeIt already uses. With the fix, next finds r.ch empty and parks in promoteSpill on spillMu, which the publisher holds. Without the fix, the Read parks on r.ch, and waitLockWaiter fails after 10 s, which is the negative control. waitLockWaiter must release r.spillMu before it calls t.Fatalf, for example through an onFail callback. Otherwise the test goroutine exits holding the lock and the parked Read leaks. Update the comment so it describes the lock-based wait and not a 50 ms budget. Kill check: mutant N3 from fix(websocket): never strand a chunk that spills after the handler drained the channel (celeris#705); fail the start helpers fast with Start's error (celeris#706) #730's body (the promotion behind the lock-free spillLen check) must still fail this case every time.

  2. The start-helper comments go stale once fix(server): a Start that never serves releases the caller's listener, the CPU monitor and the settle re-opener (#737) #747 (server: a Start that fails before its engine runs leaves the caller's listener and the CPU monitor's /proc/stat descriptor open #737) is on the branch (found by the merge train when it brought in main). fix(server): a Start that never serves releases the caller's listener, the CPU monitor and the settle re-opener (#737) #747 made a Start that fails before its engine runs close the caller's listener (prepareWithListener), and made doPrepare close the CPU monitor when createEngine fails. These comments still say that a failed Start closes neither:

    • middleware/websocket/engine_linux_test.go:80-83 (startNativeServerWithHandle)
    • middleware/websocket/engine_start_test.go:56-57
    • middleware/static/start_helper_linux_test.go:49-50
    • middleware/static/retained_key_linux_test.go:105

    The helpers' own _ = ln.Close() and Shutdown still do no harm, because both releases are idempotent. They are now a second line of defence, not the only release. Reword the comments, and decide whether each explicit release stays as a guard. The readiness-timeout path, where Start is still running when the wait gives up, is the case where the helper's cancel and wait still matter.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/testTesting infrastructuretestingTesting infrastructure and helpers

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions