Make CompressionStream native - #31728
Conversation
|
Updated 10:07 AM PT - Jun 23rd, 2026
❌ @robobun, your commit 0e0affc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31728That installs a local version of the PR into your bun-31728 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd a synchronous compression TransformStream helper; extend TransformStream internals to support cancel/finishPromise and robust teardown; migrate CompressionStream/DecompressionStream to use the helper (removing node:stream adapters); add tests for compression ordering, error propagation, and transformer.cancel semantics. ChangesStream Compression/Decompression Refactoring
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/builtins/TransformStreamInternals.ts`:
- Around line 365-370: The close() function only runs after
flush()/engine.errored paths, leaving the zlib engine open if
controller.enqueue() throws or the writable is aborted; modify the teardown so
close() is invoked on all non-flush teardown paths (readable cancel and writable
abort) by wiring readable cancel and writable abort handlers to call close()
and/or reuse the internal transform helper routines used by drive()/flush(),
ensuring engine.close() is always executed regardless of where the transform
fails (refer to close(), drive(), flush(), controller.enqueue(), and
engine.errored to locate the spots to update).
- Around line 375-377: The drive() loop in createCompressionTransform(engine)
rejects plain ArrayBuffer; normalize chunks by checking chunk instanceof
ArrayBuffer before ArrayBuffer.isView so bare ArrayBuffer becomes a Buffer; also
ensure the zlib engine is always closed on error/cancellation by wrapping
controller.enqueue(...) and the drive/flush/finish paths with try/finally (or
add a centralized cleanup that calls engine.close()) so engine.close() runs if
enqueue throws or the transform is aborted, and remove reliance solely on
engine.errored/explicit close after flush.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfe7175a-ab44-43af-984a-efdb760b13af
📒 Files selected for processing (4)
src/js/builtins/CompressionStream.tssrc/js/builtins/DecompressionStream.tssrc/js/builtins/TransformStreamInternals.tstest/js/web/streams/compression.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun fix reviews & ci - also make sure we have good test coverage for Node.js compatibility. think about edge cases |
|
(re/ compatibility, we want to make sure that even errors are the same, including when theyre thrown - e.g. async or not) |
|
✅ Fully native on the stacked base (#32595 All 34 review threads resolved. CI build 64240 on 0e0affc: 284/286 green. The two red lanes are darwin-26-aarch64: one is a buildkite artifact-download timeout (test runner never ran), the other is |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/builtins/TransformStreamInternals.ts`:
- Around line 433-465: In transformStreamDefaultSourceCancelAlgorithm, avoid
calling .$call on a possibly-cleared cancelAlgorithm by first reading
cancelAlgorithm into a local (e.g., const cancelAlgorithm =
$getByIdDirectPrivate(controller, "cancelAlgorithm")), then if cancelAlgorithm
is undefined set cancelPromise to a resolved Promise (e.g., Promise.resolve())
instead of calling .$call, otherwise call cancelAlgorithm.$call(undefined,
reason); keep the rest of the flow (clearing algorithms via
$transformStreamDefaultControllerClearAlgorithms, wiring cancelPromise.$then
handlers, and resolving/rejecting promiseCapability) unchanged so behavior
matches the sink abort path and prevents a crash when algorithms were cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb1388f2-9ad1-434d-85fc-e6df342a22b7
📒 Files selected for processing (5)
src/js/builtins/BunBuiltinNames.hsrc/js/builtins/TransformStream.tssrc/js/builtins/TransformStreamInternals.tstest/js/web/streams/compression.test.tstest/js/web/streams/streams.test.js
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
- This PR has far too many changes to TransformStream internals to support this one particular usage of it. It's an important usage, but I think this going to cause bugs and I'm hesitant to make a large change to this without running web platform tests in CI
- CompressionStream & DecompressionStream really shouldn't be using node:zlib internally. They should get their own bindings specific to the use-case. And yes, synchronous makes sense for very tiny inputs but for large inputs it will certainly be a regression for main thread performance.
- The particulars here - of using Buffer and all the tiny allocations - probably don't make sense. But doing it as a natively-managed Vec will be challenging to properly inform the GC of the memory cost so it shows up in reporting (it would need to both call reportMemoryAllocated and reportMemoryVisited, behind a mutex since GC happens) and would need a bunch of tests
|
Re: the TransformStream internals concern — for sizing the risk, the non-compression part of the delta is one self-contained spec feature: the The CompressionStream rework direction (dedicated native bindings, threadpool for large inputs, Vec-backed output with |
|
Let's do the most idiomatic, proper solution, which sounds like correctly moving the code to native. The tests are correctly pinning and match WPT, so keep at least those. Consider even more coverage, too, including things like checking for leaks. |
|
Done — the implementation is now fully native. Two commits:
Release-build numbers (16MB gzip payload, same machine):
Coverage: compression.test.ts 38/38 (incl. 3 new engine-lifecycle leak tests: completed / cancelled mid-stream / GC-abandoned, RSS-bounded), streams dir green (the one cat-pipe RSS failure is the pre-existing ASAN-baseline threshold issue, unrelated — it never constructs a compression stream), both node parallel webstreams files pass. |
alii
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/streams/compression.test.ts`:
- Line 289: Replace the swallowed writer.close() call with an explicit await and
assertion: after the drain step completes, await the writer.close() promise and
assert its settlement (e.g., use the test framework's "resolves"/"rejects"
helpers or await and let a rejection fail the test) instead of using .catch(()
=> {}), so any unexpected close failure fails the test and is captured for
diagnosis.
- Line 294: Replace the `"hello".repeat(8)` usage with a Buffer.alloc-based
construction as per the test guideline: in the assertion that calls
expect(total.subarray(64 * 1024).toString()).toBe(...), change the RHS to
Buffer.alloc(5 * 8, "hello").toString() (or Buffer.alloc(40,
"hello").toString()) so the repetitive string is built via Buffer.alloc rather
than String.prototype.repeat.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe1fcdb9-8e4e-4dd6-b098-a59546027613
📒 Files selected for processing (14)
packages/bun-types/bun.d.tspackages/bun-types/globals.d.tssrc/js/builtins/BunBuiltinNames.hsrc/js/builtins/CompressionStream.tssrc/js/builtins/DecompressionStream.tssrc/js/builtins/TransformStreamInternals.tssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/generated_classes_list.rssrc/runtime/webcore.rssrc/runtime/webcore/CompressionStreamTransformer.rssrc/runtime/webcore/compression.classes.tstest/js/web/streams/compression.test.tstest/js/web/streams/streams-leak.test.tstest/js/web/streams/streams.test.js
|
@robobun fix review comments |
|
@Jarred-Sumner status on each point of your review, since the implementation changed substantially after it: node:zlib internals. CompressionStream/DecompressionStream now have their own binding, Buffer and tiny allocations. Gone. Output chunks are exact-size native allocations adopted by JS as Uint8Arrays (full 16KB windows are zero-copy handoffs), so the GC sees every output byte through normal ArrayBuffer accounting, and the fixed engine context footprint is reported via TransformStream internals. The non-compression delta is one self-contained spec feature, Sync for large inputs. This is the one open point. A single huge chunk does compress synchronously on the JS thread now (16MB gzip measures 5.4ms compress / 3.7ms decompress in release; the old adapter paid a threadpool round-trip per 16KB of output even for tiny writes). For large writes I would add threshold-based offload reusing the node:zlib WorkPool pattern: copy the input, run the same drive loop on the worker, |
|
@robobun rebase |
d8e182a to
1f4b0b6
Compare
b1004c9 to
800f5f5
Compare
2113e7e to
2dfdbaa
Compare
800f5f5 to
aa1f955
Compare
d58e90c to
873cbdc
Compare
|
@robobun adopt |
|
@alii not sure which sense of "adopt" you mean here. The rebased/reordered stack at 873cbdc looks right (534cc2a + the tamper-proof fix now sit under the |
Adds the transformer.cancel(reason) lifecycle hook from whatwg/streams#1283: fires on reader.cancel()/writer.abort(), mutually exclusive with flush, gates the teardown promise on its return. Wires [[cancelAlgorithm]]/[[finishPromise]] through the controller and rewrites the sink-abort/source-cancel algorithms to the post-#1283 spec text. Guards three teardown races where the spec reference implementation crashes (write-vs-cancel, terminate-then-cancel, abort during a failing transform) so user-facing promises settle with the correct reason. Carved out of #31728 so that PR's TransformStream-internals delta drops to zero. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
…iming Adds test/js/third_party/wpt-streams/ with the upstream streams/transform-streams/cancel.any.js (byte-identical, vendored at wpt@e4a4672e9e) driven by the existing testharness shim, so the actual WPT suite for transformer.cancel runs in CI. Running it found one spec-timing bug not caught by the hand-mirrored tests: the constructor resolved startPromise via an extra Promise.resolve().then() hop instead of resolving it directly with the start() result (spec step 14). That delayed the writable's [[started]] flip by one microtask, so a controller.error() inside transformer.cancel() left the writable in "erroring" (not yet "errored") when the source-cancel fulfill reaction checked it — readable.cancel() then fulfilled instead of rejecting with the controller error. WPT "readable.cancel() and a parallel writable.close() should reject if a transformer.cancel() calls controller.error()" pins this. 11/11 WPT cancel tests now pass. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
8a2d38b resolved startPromise synchronously to make one cancel.any.js case pass, but vendoring the rest of the transform-streams WPT suite showed that change shifts [[started]] one microtask too early for three other tests (errors.any.js / general.any.js readable.cancel()-then-controller.error() ordering). The original extra hop was compensating for a deeper divergence: Web IDL "a promise resolved with x" is `new Promise(r => r(x))` (always a fresh promise — the spec ref-impl explicitly avoids Promise.resolve for this reason), but writableStreamDefaultControllerStart uses Promise.$resolve which returns the input promise unchanged. Fixing that is a WritableStream change outside this PR's scope; for now revert the TransformStream constructor to its previous form and mark the one timing-dependent cancel.any.js case as a documented known failure (10/11 WPT cancel tests pass). Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
… test With the Web IDL promise-resolved-with fix underneath (the writable's [[started]] reaction now queues at the spec hop), all 11 cancel.any.js WPT cases pass. The hand-mirrored "failing flush racing reader.cancel()" test asserted the old hop-count's outcome (close runs flush before cancel joins); with spec timing, cancel reaches the source-cancel algorithm first and close joins its finishPromise — flush is never invoked, matching Node. Rewrite the test to trigger reader.cancel() from inside flush(), which is the scenario the SinkCloseAlgorithm reject-with-r change actually guards. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
Replaces the node:zlib-adapter implementation with a dedicated CompressionStreamTransformer native class (the TextEncoderStreamEncoder pattern) owning the same streaming zlib/brotli/zstd contexts node:zlib uses, and the whole drive loop: transform(chunk, isFinish) runs the consume/produce loop in Rust and returns exact-size adopted output Uint8Arrays (full output windows handed to JS with no copy). The JS builtin is only type coercion, error wrapping, and enqueue; no node:zlib stream object, no Duplex, no threadpool, no JS drive loop. Also implements the spec transformer.cancel hook (whatwg/streams#1283): cancelAlgorithm/finishPromise on the controller and the spec text for the source cancel and sink abort/close algorithms, with the WPT transform-streams cancel cases mirrored as bun tests. The compression builtin uses it to release the native context promptly on reader.cancel() / writer.abort(). Rebased onto main with the Node v26 chunk-type semantics from #31991: plain ArrayBuffer is now accepted (wrapped as a Uint8Array for the native transform), SharedArrayBuffer and SAB-backed views reject with ERR_INVALID_ARG_TYPE. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
format in modes walks the prototype chain, so Object.prototype keys like "toString" passed the check and reached the native constructor's number guard with the wrong error code. __proto__: null restricts the in check to own keys.
constructor() returning Err after Box<Self> is built runs Drop, which calls Engine::close() on a context whose init failed with state: None. brotli's close() unwraps that and zstd's passes null to ZSTD_CCtx_reset. Set Engine::Closed inside the init closure on error so Drop is a no-op and the intended JS exception propagates.
Chunks past 64KB (4× the 16KB output granularity) take a new transformAsync path: input is copied, the same drive loop runs on the work pool via AnyTaskJob, and the write resolves once the worker completes. Small chunks stay synchronous (no copy, no thread hop, no promise allocation). The engine stays in place — z_stream is self-referential and must not move — guarded by write_in_progress / pending_close flags exactly as NativeZlib does, with close() deferring until the in-flight job returns. Output is byte-identical to the synchronous path and to node:zlib's defaults. Addresses the remaining review point on this PR: synchronous compression of a single huge chunk no longer holds the JS thread. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
The input-size threshold is the right axis for compression, but for decompression a sub-threshold compressed chunk can expand to arbitrary output and stall the JS thread for the whole drive loop. Route the five decode modes through transformAsync regardless of input size; the pre-PR node:zlib adapter was always async here too. Keeps the encode sync fast path. Also: settle the transformAsync promise when building the output array throws instead of leaving the awaiting write hung, and delete the now-uncalled newBufferSourceTransformPairFromDuplex adapter.
Quarantine + shadow memory push absolute RSS past any fixed budget on the ASAN debug build (~1.4GB observed vs the 700MB ASAN budget). Skip rather than keep widening the budget; the relative-growth property is asserted in release CI. Drop the now-dead ASAN budget branches. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
newBufferSourceTransformPairFromDuplex was the only thing that ever set these options; with CompressionStream now native the symbol definitions, option reads, writableOptions passthrough, try/catch wrapper, exports and the test comment referencing them are all dead.
brotli at the default quality 11 only buffers input during BROTLI_OPERATION_PROCESS; the residual block (up to ~256KB) is encoded at BROTLI_OPERATION_FINISH. flush() passes a 0-byte chunk which was always below the async threshold, so a 200KB brotli stream's finish ran ~330ms of q11 entropy coding on the JS thread. The pre-PR adapter ran this on the threadpool. flush() now calls transformAsync directly and chains close() on its settlement. The one thread hop is noise for zlib/zstd whose finish is just a trailer.
reader.cancel() arriving while the finish-flush worker is in flight closes the readable and short-circuits sourceCancelAlgorithm on the already-set finishPromise, so the transformer's cancel() hook never runs. The worker then resolves and enqueueOutputs throws on the closed readable, which skipped close() (engine lingered until GC) and rejected both writer.close() and reader.cancel() with the internal 'cannot close or enqueue' TypeError. Close first (the engine is done once the outputs are extracted) and swallow the enqueue throw: the outputs have nowhere to go and sinkCloseAlgorithm resolves the close promise cleanly since the readable is closed, not errored.
At the default quality 11, BROTLI_OPERATION_PROCESS only buffers input until the encoder's ring buffer reaches input_block_size (~256KB) and then entropy-codes the whole metablock in that call. A stream of sub-64KB writes (the common pipeThrough case from network or file sources) took the synchronous path on every chunk and ran the ~300ms q11 encode on the JS thread each time cumulative input crossed a 256KB boundary. zlib and zstd encode compress incrementally per PROCESS call so their input-size threshold stays. Also fixes the 'reader.cancel() while a large write is in flight' test: it called writer.write(big) before the writable controller's started flag was set (a microtask after construction), so the chunk was only queued and transformAsync never ran; pending_close was never exercised. Await writer.ready first so the work-pool job is actually in flight when cancel lands.
aa1f955 to
2caa760
Compare
873cbdc to
0e0affc
Compare
Adds the transformer.cancel(reason) lifecycle hook from whatwg/streams#1283: fires on reader.cancel()/writer.abort(), mutually exclusive with flush, gates the teardown promise on its return. Wires [[cancelAlgorithm]]/[[finishPromise]] through the controller and rewrites the sink-abort/source-cancel algorithms to the post-#1283 spec text. Guards three teardown races where the spec reference implementation crashes (write-vs-cancel, terminate-then-cancel, abort during a failing transform) so user-facing promises settle with the correct reason. Carved out of #31728 so that PR's TransformStream-internals delta drops to zero. Claude-Session: https://claude.ai/code/session_01Qn8dDrArUZxc4towq11qzu
6aeb0d8 to
40f5bac
Compare
3 participants
What does this PR do?
Makes
CompressionStream/DecompressionStream~4.7x / ~1.6x faster by replacing the node:zlib-adapter implementation with a native transformer class, and offloads large chunks to the work pool so single huge writes no longer hold the JS thread.Before: the constructor built a node:zlib Duplex and wrapped it with
Readable.toWeb+Writable.toWeb. Every chunk took the async write path — one threadpool round-trip per 16KB of output — plus the Node Duplex machinery and a full copy of every output chunk in the Readable adapter.After: a native
CompressionStreamTransformerclass (theTextEncoderStreamEncoderpattern) owns the same streaming zlib/brotli/zstd contexts node:zlib uses and the drive loop:transform(chunk, isFinish)runs the whole consume/produce loop in Rust and returns ≤16KB output chunks, each its own adopted allocation — full output windows hand their buffer to JS with no copy, and separate allocations mean no shared backing store for a consumer to reach past. The JS side of the builtin is only type coercion, error wrapping, and enqueue — no node:zlib stream object, no Duplex, no JS drive loop. The deadnewBufferSourceTransformPairFromDuplexadapter and itskValidateChunk/kDestroyOnSyncErrorhooks are deleted.Work-pool offload: chunks past 64KB call
transformAsyncinstead — input is copied, the same drive loop runs on the work pool viaAnyTaskJob, and the write resolves once the worker completes. Decompression and brotli encode go throughtransformAsyncregardless of input size (a sub-threshold compressed chunk can expand to arbitrary output; brotli encode is slow at every size). The finish-flush also runs on the work pool. The engine stays in place —z_streamis self-referential and must not move — guarded bywrite_in_progress/pending_closeflags exactly asNativeZlibdoes, withclose()deferring until the in-flight job returns. Output is byte-identical to the synchronous path and to node:zlib's defaults.Behaviors deliberately preserved (pinned by tests):
TransformStream(readablehighWaterMark: 0) stalls the first write until a reader attaches; the previous implementation resolved writes immediately while buffering, and code in the wild relies on that. The readable side keeps a byte-counting strategy with one chunk of headroom.ArrayBufferandnullrejected with the same error codes;ERR_INVALID_ARG_VALUEon unknown formats; corrupt input rejects with the engine's code (Z_DATA_ERRORetc.) and message. All five formats (gzip/deflate/deflate-raw/brotli/zstd), byte-identical output (same engine defaults).transformer.cancelreleases the native context onwriter.abort()/reader.cancel(); errors and the finish-flush close it (before enqueueing the final outputs, so a teardown racing the last enqueue cannot reach a live context); GC-abandoned streams release it in the finalizer.Also fixed along the way: writing a TypedArray over a detached ArrayBuffer now throws a TypeError (previously relied on
Buffer.fromthrowing; an explicit check pins it).Measured (16MB gzip payload, release builds, macOS arm64)
DecompressionStreamgunzipSync(was 6.7x)CompressionStream(level 6)gzipSyncparityTests
test/js/web/streams/compression.test.ts: 53 pass — round-trips for all five formats, input-type/error-code pins, write-before-read ordering, corrupt-input rejection, cancel/abort teardown, detached-buffer rejection, engine-lifecycle leak coverage (completed / cancelled mid-stream / GC-abandoned, RSS-bounded), and the work-pool path: 256KB single-write round-trips for all five formats, byte-identical togzipSync, sequential gating,reader.cancel()while a large write is in flight, corrupt-large-chunk rejection.test-whatwg-webstreams-compression.jsandtest-global-webstreams.jspass. Two pins that asserted the old shared-backing-buffer strategy itself (zeroed reachable tail, buffer reuse) now assert the strictly stronger property: output chunks havebyteOffset === 0and no reachable tail at all.Stack
Stacked on #32595 (
transformer.cancelspec hook), which is stacked on #32620 (Web IDLpromiseResolvedWithsemantics). Against that base this PR's delta toTransformStreamInternals.ts/TransformStream.tsis thecreateCompressionTransformbuiltin only — zero changes to the spec algorithms. #32601 (full transform-streams WPT suite, 132/133) sits alongside.