Skip to content

middleware/websocket: a pause decided on a stale depth snapshot wedges the connection permanently when the handler drains to empty first (distinct from #667) #672

Description

@FumingPower3925

Summary

chanReader.requestPause decides to pause from a depth snapshot. If the handler drains the
channel to EMPTY before that pause is applied to the engine, the reader ends up paused with
nothing buffered — and Read only re-evaluates the resume after a successful dequeue, which
can never happen again because the engine is paused and will deliver nothing more. The
connection wedges permanently.

This is a stale-pause / lost-wakeup, and it is distinct from #667. #667 is a reordering
defect: two intents decided in one order and applied in the other. This one fires even when the
order is correct — the pause is simply decided against state that no longer holds by the time it
lands. Fixing #667 does not fix this, and this is present with or without that fix.

Found while A/B-ing the #667 fix (PR #671). Not fixed there, and deliberately not smuggled into
it.

Evidence

A SIGQUIT goroutine dump taken while the benchmark was stalled shows the consumer parked in
Read's blocking select at middleware/websocket/engineread.go:313 while the producer spun with
desired == true — i.e. the engine believes recv is paused, the reader is blocked waiting for
data that the pause guarantees will not arrive, and nothing will re-evaluate.

The rate is not incidental. In the #671 A/B apparatus the escape-hatch counter forced/op
distinguishes a wedged run from a healthy one, and at cap16 the wedge degenerated 9 of 20
samples at 8 MiB memlock and 13 of 20 at 128 MiB on the unfixed arm (edge rate collapsing
~40x, forced/op 0.26-1.0 against ~1e-4 healthy). The benchmark only completes at all because it
carries a documented escape hatch; production has none.

Why it needs its own fix

The resume decision has to be reachable from a state where the buffer is empty and the engine is
paused. Candidate directions, to be judged by measurement rather than by argument:

  • Re-evaluate the watermark after applying the pause, under the same lock, and resume
    immediately if the depth has since fallen below lowWater.
  • Make Read re-evaluate the resume condition on entry, not only after a successful dequeue, so
    a reader that arrives to an empty buffer while paused can still release the engine.

Either way the failing-first test should assert the wedge directly — park the pause until the
handler has drained to empty, then require that the connection still delivers — and must not
decide anything by timing (see the #667 tests in PR #671 for the shape: a happens-before edge
forced by a channel, with anti-vacuity guards on the callback counts).

Interaction with PR #671

In the like-for-like healthy comparison, forced/op is roughly 3x higher on the #671 fix arm
in both batches. That is the review's open question on that PR: whether taking the callbacks
inside pausedMu makes this wedge more likely by widening the window in which the depth can
change between the decision and its application. It should be settled before or alongside this
fix, not after.

No activity

Activity on this issue will appear here.

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 workingmiddlewareMiddleware implementation

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions