fix(runtime): honor promisify.custom on stream.pipeline/finished (#6692) - #6694
Conversation
…ryTS#6692) Node defines `stream.pipeline[util.promisify.custom]` and `stream.finished[util.promisify.custom]` pointing at the promise-based `node:stream/promises` implementations, so `promisify(stream.pipeline)` returns that impl (a `(...streams) => Promise` with no trailing callback). Perry never installed those hooks, so `promisify(stream.pipeline)` fell back to the generic callback-appending wrapper. Bundled node-fetch (pi's fd downloader) does `const pipeline = promisify(Stream.pipeline); await pipeline(body, dest)` expecting Node's custom promise impl; routing it through the generic wrapper instead let the stream args be dropped under some dispatch paths, surfacing as `ERR_MISSING_ARGS` ("The \"streams\" argument must be specified"). Install the `promisify.custom` symbol on the `stream.pipeline` / `stream.finished` bound-native exports, pointing at the corresponding `stream/promises` callables. The existing `custom_promisified_value` path in util_promisify.rs then honors them, so `promisify(...)` returns the dedicated, well-tested promise-pipeline instead of the generic wrapper — matching Node and sidestepping the fragile forwarding path. - callable_exports.rs: wire the custom hook when minting the pipeline / finished exports (GC-rooted; no-ops if stream/promises is unavailable). - node_submodules/mod.rs: `stream_promises_export_callable` helper that installs the submodule registry entry and returns its export value. Adds test_gap_promisify_pipeline_custom_6692 (byte-matches Node 26.5.0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe runtime adds ChangesStream promisify integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant util_promisify
participant stream_pipeline
participant stream_promises
Test->>util_promisify: promisify stream.pipeline
util_promisify->>stream_pipeline: read util.promisify.custom
stream_pipeline-->>util_promisify: return promise implementation
Test->>util_promisify: invoke with streams
util_promisify->>stream_promises: run promise-based pipeline
stream_promises-->>Test: resolve undefined
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 `@crates/perry-runtime/src/object/native_module/callable_exports.rs`:
- Around line 81-88: Propagate the relocated pointer from
attach_stream_promisify_custom: at
crates/perry-runtime/src/object/native_module/callable_exports.rs:81-88, shadow
value with the function’s returned f64; at
crates/perry-runtime/src/object/native_module/callable_exports.rs:228-260,
change attach_stream_promisify_custom to return f64 and return
target.get_nanbox_f64() after allocating operations so the caller receives the
updated pointer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1b51b46-8d2e-48bb-a577-08212353fadd
📒 Files selected for processing (3)
crates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/object/native_module/callable_exports.rstest-files/test_gap_promisify_pipeline_custom_6692.ts
…tom attach CodeRabbit: attach_stream_promisify_custom roots the receiver in a handle scope and then allocates (stream/promises export + custom symbol), which can trigger a GC that evacuates the closure. Only the scope handle tracked the move, so the caller's `value` local could go stale before it is stored in NATIVE_CALLABLE_EXPORTS. Return the possibly-relocated pointer and have the caller adopt it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes #6692. Node defines
stream.pipeline[util.promisify.custom]andstream.finished[util.promisify.custom], pointing at the promise-basednode:stream/promisesimplementations. Sopromisify(stream.pipeline)in Node returns a(...streams) => Promisewith no trailing callback.Perry never installed those hooks, so
promisify(stream.pipeline)fell back to the generic callback-appending wrapper. The bundled node-fetch that pi uses to downloadfddoes:Routing that through the generic wrapper (instead of Node's dedicated promise-pipeline) is where the stream args got dropped under some dispatch paths, surfacing as
TypeError: The "streams" argument must be specified(ERR_MISSING_ARGS).Fix
Install the
promisify.customsymbol on thestream.pipeline/stream.finishedbound-native exports, pointing at the correspondingstream/promisescallables. The existingcustom_promisified_valuepath inutil_promisify.rsthen honors them, sopromisify(stream.pipeline)returns the dedicated, well-tested promise-pipeline (thunk_streamP_pipeline, registered restfixed_arity = 2) instead of the generic wrapper — matching Node exactly and sidestepping the fragile forwarding path entirely.object/native_module/callable_exports.rs: wire the custom hook when minting thepipeline/finishedexports (GC-rooted via a handle scope; no-ops ifstream/promisesis unavailable, leaving the generic fallback in place).node_submodules/mod.rs:stream_promises_export_callablehelper that installs the submodule registry entry (so the lookup succeeds even when the program never importedstream/promises) and returns its export value.Behavior parity (Node 26.5.0)
typeof stream.pipeline[Symbol.for("nodejs.util.promisify.custom")]undefinedfunctiontypeof stream.finished[Symbol.for("nodejs.util.promisify.custom")]undefinedfunctionpromisify(pipeline)(src, dst)undefinedTesting
test-files/test_gap_promisify_pipeline_custom_6692.ts— byte-matches Node 26.5.0 (custom-symbol presence, promisified pipeline with a transform stage, promisifiedfinished). Verified under both the fast dev build and the full auto-optimize / LTO (release) path.test_parity_stream_promises,test_issue_3070_stream_promises_input_validation,test_issue_1856_1857_exports_promisifystill byte-match Node.cargo test -p perry-runtime --libstream_promises / native_module_stream suites pass.Notes
ReadableStreampassed as a pipeline body isn't recognized by thestream/promisesbody validator — after this change it rejects withERR_INVALID_ARG_TYPE(previously the generic path silently hung). Both diverge from Node, which accepts web streams; that's a distinct web-stream-support gap.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
util.promisifywhen promisifyingstream.pipelineandstream.finished.Bug Fixes
pipeline/finishedproperly hook into the correspondingstream/promisesimplementations, with correct handling for unknown exports.Tests
util.promisify.customforstream.pipeline(including transform stages) andstream.finishedpromise resolution behavior.