Skip to content

Settle a TS compression write or close cut short by a cancel - #7645

Merged
jasnell merged 2 commits into
mainfrom
jasnell/ts-streams-compression-drain-cancel
Oct 9, 2026
Merged

jasnell merged 2 commits into
mainfrom
jasnell/ts-streams-compression-drain-cancel

Conversation

@jasnell

@jasnell jasnell commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Under TS a compression pair moves its output into the readable inside
the write or close that produced it, in 64 KiB pieces. Resolving a
waiting read looks up then on its result, so an Object.prototype.then
getter can cancel the reader in the middle of that drain. The drain then
stopped, and the write or close rejected with the cancel reason. With a
gzip pair, a pending read and an Object.prototype.then getter that
cancels the reader, TS / C++ / Node:

write: rejected with the reason / resolved / resolved
close: rejected with the reason / resolved / rejected

A spec TransformStream whose transform or flush enqueues the same output
resolves both. After a cancel during the transform the writable errors
with the reason; after one during the flush, close, writer.closed and
the cancel all resolve. That is what TS's own TransformStream and Node's
do. Node's compression streams are a zlib Duplex adapter, not a
TransformStream.

How a transform splits its output into chunks is implementation-defined,
so a drain cut short now settles as a transform that enqueued all its
output as one chunk before the cancel: the rest is dropped and the write
resolves, after which the writable is errored as before (compression
ledger #13). The close resolves too, as do writer.closed and the
cancel; for the close this is parity with C++. A readable errored during
the drain (the Node.js interop hook) still rejects the close with its
error, as the spec's flush does.

Pins: compression reentrancy.js cancelFromReadResultThenGetterDuringWrite
now expects the write to resolve in both implementations (TS: the
writable errors with the reason; C++: untouched);
cancelFromReadResultThenGetterDuringClose (parity: close, writer.closed
and the cancel resolve, the read the getter fired on still gets the
tail); interopErrorFromReadResultThenGetterDuringClose (TS only: the
close rejects with the hook's error). Without the source change, the
first two fail, in the TS cells only; the third passes either way and
guards the errored-readable branch.

@jasnell
jasnell added this pull request to stack #7531 October 6, 2026 21:50
@jasnell
jasnell requested review from a team as code owners October 6, 2026 21:50
@jasnell
jasnell requested review from guybedford and npaun October 6, 2026 21:50
@ask-bonk

ask-bonk Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@jasnell Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@jasnell
jasnell force-pushed the jasnell/ts-streams-compression-drain-cancel branch from d00aa99 to 8c5370c Compare October 7, 2026 22:18
@ask-bonk

ask-bonk Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Since last review: 1 resolved, 0 still open, 0 new.
LGTM!

Not re-run: api-compat, docs, compat-flags, design-simplicity (no author changes in their files since the last review)


Reviewed commit: 348d9ecc · github run

Comment thread src/tests/streams/compression/reentrancy.js
@codecov-commenter

codecov-commenter commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.60%. Comparing base (4018249) to head (348d9ec).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7645   +/-   ##
=======================================
  Coverage   38.59%   38.60%           
=======================================
  Files         868      868           
  Lines      268583   268608   +25     
  Branches    25422    25426    +4     
=======================================
+ Hits       103665   103687   +22     
  Misses     150704   150704           
- Partials    14214    14217    +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasnell
jasnell force-pushed the jasnell/ts-streams-compression-drain-cancel branch 6 times, most recently from 63be3e4 to 3db618e Compare October 8, 2026 23:23
Base automatically changed from jasnell/ts-streams-decoder-shared-buffer to main October 9, 2026 12:08
Under TS a compression pair moves its output into the readable inside
the write or close that produced it, in 64 KiB pieces. Resolving a
waiting read looks up `then` on its result, so an Object.prototype.then
getter can cancel the reader in the middle of that drain. The drain then
stopped, and the write or close rejected with the cancel reason. With a
gzip pair, a pending read and an Object.prototype.then getter that
cancels the reader, TS / C++ / Node:

  write:  rejected with the reason / resolved / resolved
  close:  rejected with the reason / resolved / rejected

A spec TransformStream whose transform or flush enqueues the same output
resolves both. After a cancel during the transform the writable errors
with the reason; after one during the flush, close, writer.closed and
the cancel all resolve. That is what TS's own TransformStream and Node's
do. Node's compression streams are a zlib Duplex adapter, not a
TransformStream.

How a transform splits its output into chunks is implementation-defined,
so a drain cut short now settles as a transform that enqueued all its
output as one chunk before the cancel: the rest is dropped and the write
resolves, after which the writable is errored as before (compression
ledger #13). The close resolves too, as do writer.closed and the
cancel; for the close this is parity with C++. A readable errored during
the drain (the Node.js interop hook) still rejects the close with its
error, as the spec's flush does.

Pins: compression reentrancy.js cancelFromReadResultThenGetterDuringWrite
now expects the write to resolve in both implementations (TS: the
writable errors with the reason; C++: untouched);
cancelFromReadResultThenGetterDuringClose (parity: close, writer.closed
and the cancel resolve, the read the getter fired on still gets the
tail); interopErrorFromReadResultThenGetterDuringClose (TS only: the
close rejects with the hook's error). Without the source change, the
first two fail, in the TS cells only; the third passes either way and
guards the errored-readable branch.

Compatibility: the change is behind the experimental
typescript_implemented_streams flag.
@jasnell
jasnell force-pushed the jasnell/ts-streams-compression-drain-cancel branch from 3db618e to 39012b0 Compare October 9, 2026 12:09
A read result's `then` getter that errors a compression pair through the
Node.js interop hook while a write is delivering leaves that in-flight
write resolving, as a spec transform's does: only the flush checks the
readable's state. Both sides then expose the hook's error. Nothing pinned
the write side of this, so restoring a throw there for an errored
readable passed the suite.

Pins: compression reentrancy.js
interopErrorFromReadResultThenGetterDuringWrite (TS only: the write
resolves, writer.closed and reader.closed reject with the hook's error).
With `if (finished && readableErrored) throw finishReason;` added after
the write's drain, it fails in the TS cell.

Also corrects the compression AGENTS.md Reads bullet, which still said
a reader cancel from the getter fails the write.
@jasnell
jasnell merged commit d08010b into main Oct 9, 2026
28 checks passed
@jasnell
jasnell deleted the jasnell/ts-streams-compression-drain-cancel branch October 9, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants