Repository navigation
Conversation
…ect as this pull(), pullSync(), pipeTo() and pipeToSync() read the transform function from a transform object and called it with no receiver. The three helpers that start a stateful transform now take the object and call the function with it as `this`, as Node.js does since nodejs/node@649a4aba93. test-stream-iter-transform-roundtrip.js is the upstream file at that commit.
|
Updated 8:49 AM PT - Sep 27th, 2026
❌ @robobun, your commit 5bc495f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 44131That installs a local version of the PR into your bun-44131 --bun |
StatusReady for a maintainer. The change is complete and the tests of this PR pass in CI. CI is red for a reason that this PR does not cause. Repro, with the script in the Notes of the PR body, run with
CI, build 121259:
Limits of this PR that a reviewer must know:
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughStateful sync and async transforms now run with their transform object as the receiver. Regression tests cover ChangesStream transform receivers
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The receiver change appears ready to merge after normal checks; no actionable merge-blocking issue is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the sibling call sites in pull.ts for the receiver change: all three stateful helpers and all five callers (sync, async, and the kValidatedTransform branch) were updated with the same parameter order, and the fused stateless paths take bare functions so no receiver applies there. The transform.transform re-read at each call site happens in the same loop iteration as the isTransformObject guard, so a non-callable cannot reach $call.
Extended reasoning...
The change threads the transform descriptor object as the this receiver through applyStatefulSyncTransform, applyStatefulAsyncTransform and applyValidatedStatefulAsyncTransform in src/js/internal/streams/iter/pull.ts, using $call per the src/js conventions, and adds a spawned -e test plus a ported upstream test function. It touches no security-sensitive surface. The inline finding is a pre-existing null-flush divergence from Node rather than a regression from this diff; approval is withheld only because verified findings are attached to the review.
|
A correction to one point of the review summary, so that nobody relies on it. A value that is not callable can reach the call.
The error is a The review thread about the |
|
@robobun wake up!! |
|
@robobun wake up!! |
|
Awake. The state of this PR:
To get a green run, retry the one failed job of build 121259. I used my one CI rerun on this PR already. If you want a full run, tell me and I push an empty commit. Not in this PR:
|
Problem
pull(),pullSync(),pipeTo()andpipeToSync()ofnode:stream/itercall thetransformfunction of a transform object withthisundefined. Node.js v26.10.0 passes the object.src/js/internal/streams/iter/pull.ts. The call sites (lines 448, 642, 644) pass onlytransform.transform. The helpers (lines 418, 539, 571) call it with no receiver.Fix
applyStatefulSyncTransform,applyStatefulAsyncTransformandapplyValidatedStatefulAsyncTransformtake the transform object and call the function with.$call(receiver, ...).test/js/node/stream/node-stream.test.jsandtest/js/node/test/parallel/test-stream-iter-transform-roundtrip.js. Both fail on bun 1.4.3. All 42test-stream-iter-*.jsfiles pass. Other cases ofnode-stream.test.jstimed out (see Notes).Background
transform()method (stateful).node:zlib/itercarry an internal marker.pull()runs them through the third helper, which skips output validation..$callis the built-inFunction.prototype.call. User code cannot replace it..bind(transform): it allocates per transform object. Consideredobject.transform(...)in the helper: it reads the property later than Node.js. No design review ran.Downsides
transformfunction sawglobalThisasthis. It now sees the transform object.nullthrowsERR_INVALID_ARG_TYPE(tracked in node:stream/iter: a stateful transform that yields the null flush signal throws ERR_INVALID_ARG_TYPE #44132).Notes
Repro, run with
--experimental-stream-iter:this is the descriptor: truethis is the descriptor: falsethis is the descriptor: falsethis is the descriptor: trueThe script of the new test prints the same JSON on Node.js v26.10.0 and on this PR. On Node.js v26.3.0 and bun 1.4.3 all four receivers are
false.What each test covers. The case in
node-stream.test.jscovers the plain async helper and the sync helper, through all four entry points. It does not cover thenode:zlib/iterpath. The upstream functiontestValidatedTransformReceiverin the vendored file covers that path. On bun 1.4.3 the vendored file exits 1 attest-stream-iter-transform-roundtrip.js:49:12. With this PR it exits 0.The vendored file is not the v26.10.0 copy. It is the upstream file at commit 649a4aba93, byte for byte (sha256
7cafd1427d25621acad8f5d6b8e4dcfd0255d0c30a970c63a96da3b5811bb884). The v26.10.0 copy adds one more function,testZstdConcatenatedFrames, from nodejs/node@678ff3561a. That function fails on main:pull(from([first, Buffer.from('junk'), second]), decompressZstd())raisesZSTD_error_prefix_unknown("Unknown frame descriptor"), and Node.js v26.10.0 gives'a'. That is a zstd decoder difference and this PR does not change it. Related: #33413.Upstream tests this PR cannot add. The same upstream commit adds
testPullStatefulTransformReceiver(test-stream-iter-pull-async.js) andtestPullSyncStatefulTransformReceiver(test-stream-iter-pull-sync.js). Bun's copies of both files match v26.3.0. The upstream files at 649a4aba93 also test behaviour that bun does not have yet. I ran each function of those files on its own against bun 1.4.3, with a 3 second limit per function. Intest-stream-iter-pull-sync.js2 of 16 functions fail. Intest-stream-iter-pull-async.js9 of 32 fail, and 2 of those 9 hit the time limit and did not reach an assertion. One function in each file is the receiver test.Found outside this task. Both upstream receiver tests yield the
nullflush signal from the transform. Bun throwsERR_INVALID_ARG_TYPEfor that. Node.js skips it since nodejs/node@5c0578cbf7 ("stream: skip null output from stateful transforms"). It is tracked in #44132. Between v26.3.0 and v26.10.0,lib/internal/streams/iter/pull.jshas 22 commits. This PR ports one. I did not compare the other 21 with bun'spull.ts.Other runs on the debug build, all pass:
test-fs-promises-file-handle-pull.js,test-fs-promises-file-handle-pullsync.js,test-fs-promises-file-handle-writer.js,test-quic-stream-iteration-pull.mjs,test-quic-stream-iteration-pipeto.mjs,test-quic-h3-post-filehandle.mjs,test-quic-stream-body-filehandle.mjs.bun lintandtsc --noEmit -p src/js/tsconfig.jsonpass.Runs of
node-stream.test.json the debug build. The machine had a load average of 600 to 800 during all runs. The new case in its final form passed in 4 of 4 runs (1.7 to 2.9 s). A first version of the case also loadednode:zlib/iterin the same process. It timed out at 5000 ms in 2 of 2 runs on this machine. That is why the case leaves thenode:zlib/iterpath to the vendored file. The full file was not green in either of 2 runs. In the second run the failures were two existing cases,createReadStream > should emit readable on endandprocess.stdin > should pipe correctly. Run alone, each of them passed once and timed out once. Neither usesnode:stream/iter.Size.
src/codegen/bundle-modules.ts --debug=OFFon the base and on this PR:InternalModuleRegistryConstants.binis 2,630,120 and 2,630,231 bytes. The moduleinternal/streams/iter/pull.jsis 19,125 and 19,236 bytes. The size report of the CI build for 245fa9f shows +0.0 KB for every target.A
transformthat is not callable at the call.transformcan be a getter that returns a function whilepull()validates its arguments and another value, for example42, when the pipeline reads the property to call it. bun 1.4.3 throwsTypeError: transform is not a function. (In 'transform(withFlushAsync(source), options)', 'transform' is 42). This PR throwsTypeError: transform.@call is not a function. (In 'transform.@call(receiver, withFlushAsync(source), options)', 'transform.@call' is 42). Node.js v26.10.0 throwsTypeError: Function.prototype.call was called on 42, which is a number and not a function. The class is the same and no error has acode. The message text changes, and it shows the internal name@call.pullSync()gives the same result. The debug build does not crash.Not done. I made no release build of this PR. My own runs used the debug build only. No instruction count was taken. No design review and no separate self-review ran. I read the diff against the upstream commit line by line.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/stream/node-stream.test.js