Skip to content

middleware/websocket: chanReader applies pause/resume to the engine outside the lock that decides them, so a resume can be lost and the connection stops delivering (found by reading, not reproduced) #667

Description

@FumingPower3925

Summary

chanReader (middleware/websocket/engineread.go) calls the engine's pause() and resume()
callbacks outside the pausedMu critical section that decides to call them. Two goroutines —
the engine thread appending inbound data and the handler goroutine draining it — can therefore
apply their intents to the engine in the opposite order from the order in which they decided
them, leaving the engine's recv paused while the reader believes it is running. Nothing ever
re-evaluates, so that connection stops delivering inbound data for the rest of its life.

Found by reading, not reproduced. Filed separately from #633 because it is engine-agnostic
(it is in the middleware, not in epoll or io_uring) and because, unlike the stall in #633, it is
deterministically testable on main today — see the test below.

Mechanism

  • requestPause (engineread.go:221-233) takes pausedMu, decides to pause, sets pausedState,
    releases the mutex, and then invokes pause().
  • Read's resume branch (engineread.go:309-316) does the same in reverse: decides under the
    mutex, clears pausedState, releases, and then invokes resume().
  • Both engine callbacks are Swap-based and ignore the previous value
    (engine/iouring/worker.go:2177-2208: the closures early-return on a no-op swap), so whichever
    callback reaches the engine last wins outright — there is no reconciliation.

Interleaving that loses a resume:

  1. Appends cross highWater; the appending goroutine leaves pausedMu and is descheduled just
    before pause().
  2. The handler drains below lowWater, takes pausedMu, sees pausedState == true, clears it,
    releases, and calls resume() — the engine is not paused yet, so this is a no-op.
  3. The appending goroutine resumes and calls pause().

Final state: engine recv paused, reader pausedState == false. The reader will never call
resume() again, because from its point of view it is not paused. The symmetric interleaving
(parking resume() instead) leaves the engine running while the reader thinks it is paused,
which is the benign direction.

Why this is being filed now

It came out of the diagnosis of the WebSocket close-handshake stalls (#633, #607, #611, #623) as
the one latent race that fits the symptom with no witness counter: every existing io_uring
witness reads zero in the captured failure (LINKBLOCK arms=0, RecvSQFull=0, armDeclined=0,
resumeWhileCancelPending=0, doubleArmed=0, cqeUnaccounted=0), which excludes #607, the SQ-full
recv-owed path and the #484 window. This race leaves no counter behind, which is consistent.

It is not claimed to be the cause of those stalls. Its measured rate is unknown and it looks
too rare to explain the observed rate on its own. It is filed as a defect in its own right.

Failing-first test (deterministic, no engine, no timing on the failing path)

TestChanReaderLostResumeConverges in middleware/websocket/engineread_test.go:

  1. r := newChanReader(8, 0, 0) (highWater 6, lowWater 2).
  2. var desired atomic.Bool; entered, release channels.
  3. pause := func(){ close(entered); <-release; desired.Swap(true) },
    resume := func(){ desired.Swap(false) } — ignoring the swap result, matching the engine.
  4. r.SetPauser(pause, resume).
  5. Appender goroutine appends 6 single bytes; the 6th crosses highWater and parks in pause().
  6. After <-entered, the handler goroutine reads 4 bytes (depth 6 -> 2, crossing lowWater).
  7. Release, join, then assert !(desired.Load() && !readerPaused(r)).

On main this fails every run: pausedMu is already released while pause() is parked, so the
reads finish, resume() is a no-op, and the pause lands afterwards. Add the symmetric test that
parks resume() and asserts desired.Load() == r.pausedState once both goroutines finish.

Proposed fix

Call r.pause() inside the pausedMu critical section in requestPause, and r.resume()
inside pausedMu in Read's resume branch. The engine's Swap order then equals the order of
pausedState transitions, and drainDetachQueue converges to the latest intent.

Lock order is pausedMu -> detachQMu (taken inside the engine closures). drainDetachQueue
holds detachQMu only for its slice swap (worker.go:4403-4406), never calls into middleware,
and pausedMu is never taken under detachQMu, so there is no cycle. The same change applies to
the epoll side.

Risk that must be measured before merging

Holding pausedMu across pause()/resume() puts detachQMu and an occasional eventfd write
inside a lock the handler takes on every chunk, and Append runs on the single io_uring worker
thread, so the contention lands there. #607 recorded serialization as the worse arm (8/12 vs
4/12), albeit with #607 still present. A/B the fix before merging it; do not merge on the
reasoning alone.

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

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions