fix(web): pull a streamed result behind a demand gate - #3124
Conversation
The stream was built with no `pull` and no queuing strategy, and every codec node is enqueued as soon as it is parsed, so the producer ran as fast as it could resolve whether or not anyone read. One slow consumer buffered the entire result in server memory — unbounded, and invisible to application code. The gate sits in the iterator wrapper the runtime already installs, which its own comment calls "the only seam where a dropped consumer can stop the producer": the same seam lets a SLOW consumer slow it. A read drives `pull`, `pull` releases one source pull. Teardown releases a parked pull too, or an aborted stream would hang on it. Measured with an async generator and no reads for 200 event-loop turns: the producer advanced by 1, where it previously tracked the turn count (and reached ~17,000 items, +19 MiB, in the wall-clock shape the issue reports). Reading 20 chunks advances it by 20, and cancelling stops it within the pull in flight. The solidjs#3112 marker for this gap comes off, per the convention the other two followed when they closed.
🦋 Changeset detectedLatest commit: 5d29277 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will degrade performance by 28.5%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | merge |
225.1 µs | 326.4 µs | -31.04% |
| ❌ | merge |
264.7 µs | 364.1 µs | -27.3% |
| ❌ | merge |
265.1 µs | 363.7 µs | -27.1% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing frenzzy:test/backpressure-gate (5d29277) with next (d6a4a52)
Footnotes
-
132 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
…e gate covers Review of the first shape found a regression I introduced. `onDone` and `onError` set `closed` directly rather than through `teardown()`, so a pull parked on the demand gate was stranded: `desiredSize` is 0 after close and null after error, both failing the `> 0` check, so the gate never reopened and the source's `finally` never ran — leaking generator, cursor and file-handle cleanup once per failed request. Every path that ends the stream now runs `finishSource()`, which closes the source and releases the parked pull. Also from review: - The gate rests on the default high-water mark of 1, and `releaseDemand` holds a single resolver, safe only because the pump is sequential. Both were load-bearing and unstated; both are now in the comment. - `!streamController` in `wantsMore` was unreachable — `start()` assigns the controller before the iterator is ever opened. - The spec header still called solidjs#3118 open, and the changeset claimed the producer "advanced by one step" where the test asserts it stays near the queue size. - The liveness assertion was `> 1` under a comment claiming it tracked the reads; a gate that resumed twice in twenty reads would have passed. Scope is now stated rather than implied: only the result itself is gated. A nested `{ items: rows() }` is pumped by the codec directly and still runs ahead — measured at 200 items over 200 idle turns against 1 for the top-level shape.
|
CI is red only on CodSpeed, and I do not think this change can be the cause — evidence rather than assertion: The regressed benchmarks cannot reach the changed code. All three are import { createStore, merge, omit } from "../../src/index.js";
CodSpeed says so itself, in the report: "Different runtime environments detected — Some benchmarks with significant performance changes were compared across different runtime environments, which may affect the accuracy of the results." All three regressions are the same benchmark family at 27-31%, which is the shape of a runner change rather than a code path. Those benchmarks are not steady. Two consecutive local runs, same branch, same build, no code change between them: Local wall-clock is a different measure from CodSpeed's simulation, so this does not explain 28% on its own — I am offering it as context, not proof. The import graph is the argument. For contrast, #3113 changed the same file and reported 136 untouched benchmarks. Everything else is green, including |
The first version of this test passed with finishSource() removed from
both codec-end paths — it never reached the defect. Two reasons, and the
second is the one worth writing down:
- the source was NESTED (`{ rows: gen(), … }`), and a nested iterable
never enters the wrapper, so nothing parked;
- a top-level source needs no sibling branch to carry the failure. The
deferred failure rides INSIDE a yielded chunk: a pending promise in the
first value resolving to an object whose getter throws. The pump
enqueues the chunk, asks for the second, parks because the consumer
stopped reading, and only then does the promise settle and onError fire
against a parked pull.
`produced` is asserted alongside the cleanup so the nested-shape mistake
cannot quietly recur: if the source never parks, the test says so instead
of passing for the wrong reason.
Verified both ways — with finishSource() in the codec-end paths the
cleanup runs, without it the assertion fails.
3f5f240 to
6e3f010
Compare
|
One honest caveat about this PR that came out of looking at the nested case (#3125), worth raising here rather than leaving for someone to find later. The gate only bites if whatever writes the response to the socket propagates backpressure into I have not measured a real adapter, so I am not claiming either way. The tests here prove the runtime-side property, which is the part this repository owns; whether it survives the last hop is a question for whichever adapter a deployment uses, and worth knowing before anyone treats this as a memory guarantee. If it turns out the common adapters buffer, the fix is still correct but the issue's framing ("one slow client buffers the whole result in server memory") would be understating where the buffering actually happens — it would move from the stream's queue to the adapter's. |
|
Re-ran it: the regression was the runner, not the change. I amended to a new SHA with byte-identical content (same tree hash, Same code, same base, opposite verdict. That matches the report's own "Different runtime environments detected" note, and the import graph said as much beforehand: the three regressed benchmarks were All checks green now. |
Third review round, both findings real. The gate was checked before `finished`, so teardown landing while a pull was in flight stranded the codec's pump: the release it fires finds nothing parked, the in-flight pull then resolves, the pump asks for the next item, `wantsMore()` is false — 0 after close, null after error — and it parks on a resolver nobody will ever call. `push()` never returns. Impossible before this PR, where `finished` was checked first. One token, and it mirrors the defect the previous round fixed: that one stranded the source, this one stranded the pump. Worth stating plainly: this half is not guarded by a test. The failure is a leaked pending promise inside seroval's pump with no outward symptom — the source still runs its `finally`, the response still ends, nothing observable differs. It was found with instrumented park/release counters, and that is the evidence it rests on. The tests also did not park at all. Both read in a tight loop, so a read request is always pending, `desiredSize` never drops and the producer never reaches the gate — deleting `pull()` outright left all eight green, including the one whose comment claimed it would catch a deadlock. The new test pauses between reads, which is what puts the producer on the gate, and reads to completion with a deadline so a gate that never reopens fails fast instead of hanging. It counts arrivals by its own key rather than by the codec's node shapes.
Its comment claimed it was the case that would catch a deadlock. It is not: it reads in a tight loop, so a read request is always pending, `desiredSize` never drops and nothing parks — the same blindness review found in the original pair. What it pins is the cancel half. The pausing test is the liveness one.
|
Merging past the CodSpeed red: the three flagged benchmarks are |
2 participants
Closes #3118.
The response stream is built with no
pulland no queuing strategy, and every codec node is enqueued the moment it is parsed, so the producer runs as fast as it can resolve regardless of whether anyone is reading. One slow consumer buffers the whole result in server memory — unbounded, and invisible to application code.The seam was already there
The iterator wrapper the runtime installs describes itself as "the only seam where a dropped consumer can stop the producer". The same seam lets a slow consumer slow it: a read drives
pull, andpullreleases exactly one source pull. Ending the stream releases a parked pull too, or the source would be stranded.desiredSize > 0means "fewer than one chunk queued" — the stream takes no queuing strategy, so it runs on the default high-water mark of 1. That default is what sets the depth, and it is load-bearing, so the comment says so.Measured
Consumer reads one chunk, then idles for 200 event-loop turns:
Counted in turns rather than wall-clock, so the assertion means the same thing on any machine. Throughput for a fast consumer draining a 20k-item stream is unchanged (~91 ms on both branches, within noise), and a non-streamed result never enters the wrapper.
Scope, stated plainly
The gate sits on the source the runtime wraps — the result itself. An async iterable nested inside the result (
{ items: rows() }) is pumped by the codec directly and is not gated: measured at 200 items over 200 idle turns, against 1 for the top-level shape. That is the ordinary shape, so it is worth knowing this fixes half the problem. Filed as #3125 rather than widened here, because this PR had already grown two review-found defects and adding surface without another pass seemed the wrong trade.A consumer that abandons a stream without cancelling now leaves the producer parked rather than running it to completion. That is inherent to backpressure and the consumer's side of the WHATWG contract, but it is a behaviour change.
And the limit worth knowing before treating this as a memory guarantee: the gate only bites if whatever writes the response to the socket propagates backpressure into
desiredSize. An adapter that pipesResponse.bodydoes; one that drains it eagerly —for await (… ) res.write(chunk)ignoringwrite's return, or buffering before writing — does not, and against that adapter this is a no-op observable only in-process. I have not measured a real adapter, so I am not claiming either way; the tests here prove the runtime-side property, which is the part this repository owns.What review found
Three rounds, and the third found the sharpest one.
The gate was checked before
finished. Teardown landing while a pull was in flight stranded the codec's pump: the release it fires finds nothing parked, the in-flight pull resolves, the pump asks for the next item,wantsMore()is false — 0 after close, null after error — and it parks on a resolver nobody will ever call.push()never returns. One token to fix, and it mirrors the previous round exactly: that defect stranded the source, this one stranded the pump.That half is not guarded by a test, and I would rather say so than imply otherwise. The failure is a leaked pending promise inside seroval's pump with no outward symptom — the source still runs its
finally, the response still ends, nothing observable differs. It was found with instrumented park/release counters, and that measurement is the evidence it rests on.The tests did not park at all. Both read in a tight loop, so a read request was always pending,
desiredSizenever dropped, and the producer never reached the gate. Deletingpull()outright — removing the entire reopening mechanism — left all eight green, including the one whose comment claimed it would catch a deadlock. The new test pauses between reads, which is what puts the producer on the gate, and reads to completion behind a deadline so a gate that never reopens fails fast instead of hanging.Earlier rounds. The first shape had a regression I introduced:
onDoneandonErrorsetcloseddirectly rather than throughteardown(), so a pull parked on the gate was stranded —desiredSizeis 0 after close and null after error, both failing the> 0check, so the gate never reopened and the source'sfinallynever ran, leaking generator and cursor cleanup once per failed request. Every path that ends the stream now runsfinishSource().Also corrected: an unreachable
!streamControllerguard, a stale#3118 (open)line in the spec header, a changeset that claimed more than the test asserts, and a liveness assertion of> 1sitting under a comment claiming it tracked the reads.Tests
The
test.failsmarker #3112 put on this gap comes off, following the convention the other two gaps used when they closed. Alongside it: the producer resumes as the consumer reads and stops when it leaves (which pins the cancel half; the pausing test is the liveness one).And the regression above: a codec failure landing while a pull is parked must still close the source.
Checked by mutation:
expected 200 to be less than 50finishSource()removed from the codec-end pathsexpected false to be truepull()gutted, so the gate never reopensthe gate parked and never reopenedThe second guard took two attempts. The first version passed with the fix removed — the source was nested, so nothing ever parked, and a nested iterable never reaches the gate. A top-level source needs no sibling branch: the deferred failure rides inside a yielded chunk, so the pump enqueues it, asks for the next item, parks, and only then does the failure land.
producedis asserted alongside the cleanup so that mistake cannot quietly recur.Full suite green: 45 files, 456 passed, 2 skipped; the spec is stable over repeated runs.
A later correction
Review caught two things in this description. The line about a test that "drains to completion" described a test commit
f2824f6had already replaced, and the claim that the resume test was the liveness assertion was wrong — it reads in a tight loop, so nothing ever parks, which is the same blindness the third round found in the original pair. The pausing test is what catches a gate that never reopens; the resume test pins the cancel half. Both the body and the test's own comment now say so.