fix: #769 — wire node:http / node:https client request through codegen dispatch#771
Merged
Conversation
TheHypnoo
added a commit
that referenced
this pull request
May 14, 2026
The cargo-test in #771's CI flagged that the new `ClientRequest.setTimeout` NativeModSig entry has no manifest counterpart. Existing class-filtered http methods (on/end/write/ setHeader) collapse to the manifest entries already covering ServerResponse / IncomingMessage / HttpServer — only setTimeout was genuinely new on the http module.
TheHypnoo
added a commit
that referenced
this pull request
May 14, 2026
CI parity on #771 surfaced \`test_node_http_client_request.ts\` as an output mismatch — the three concurrent requests fired their \"status\" prints in tokio-scheduling-dependent order, which diverged from Node's behavior run-to-run. Chain the requests instead (req2 inside req1's response callback, req3 inside req2's) so the line order is deterministic and matches \`node --experimental-strip-types\` byte-for-byte. Also dropped the per-path \`statusCode\` setter from the in-process server — it was unused (Perry and Node both default to 200) and only confused the expected output. Verified locally: Perry and Node both produce: server listening req1 status: 200 req2 status: 200 req3 status: 200 done
…rough codegen dispatch `http.request(...)` and `http.get(...)` (and the `https.*` variants) always returned `undefined` from compiled TypeScript, even though the runtime implementations existed in `perry-ext-http`. The FFI symbols were declared but no `NativeModSig` dispatch entries existed, so the call site fell through to the unknown-method path and got back `TAG_UNDEFINED`. Reported in #767 (case 3) and tracked separately in #769. Changes: - `perry-codegen/src/lower_call.rs`: add dispatch entries for `http.request`, `http.get`, `https.request`, `https.get` (module-level) and ClientRequest instance methods (`on`, `end`, `write`, `setHeader`, `setTimeout`). - `perry-hir/src/lower.rs` + `perry-hir/src/lower_decl.rs`: tag `const req = http.request(...) | http.get(...) | https.*` bindings as `("http", "ClientRequest")` native instances so `req.on/.end/...` dispatch through the class-filtered entries above. Mirrors the existing `net.createConnection` / `tls.connect` tagging; the closure-body path in `lower_decl.rs` had no equivalent of the top-level arm in `lower.rs`, so `request()` inside a callback was unreachable. - `perry-ext-http/src/lib.rs`: - accept either a URL string or an options object in `request_common` (was options-only), matching Node's `http.request(url, cb)` overload which the reporter's repro used; - rewrite `dispatch_request` to spawn the reqwest future as a detached task on the same multi-thread runtime instead of `Handle::current().block_on` (which panicked with "Cannot start a runtime from within a runtime" now that the dispatch reaches this code). Mirrors `perry-ext-net::spawn_socket_runner`, including the LTO `black_box` workaround for tokio's CONTEXT statics. - `perry-stdlib/Cargo.toml` + `perry-stdlib/src/common/async_bridge.rs`: add `external-http-client-pump` feature so perry-stdlib's main-thread pump drains perry-ext-http's response/error queue and keeps the event loop alive while a request is in-flight. Mirrors the existing `external-http-server-pump` plumbing. - `perry/src/commands/compile/optimized_libs.rs`: activate `external-http-client-pump` when the well-known flip routes `node:http` / `node:https` to perry-ext-http. Test: `test-files/test_node_http_client_request.ts` exercises all three forms (`request(url, cb)`, `request(options, cb)`, `get(url, cb)`) against an in-process `createServer`, verifying the response callback fires and the server actually receives the requests. Scope: response-side property access (`res.statusCode`, `res.headers`) still needs the handle-property dispatch surface and is left as a follow-up — the repro from #769 only needs `req.on('error', ...)` and `req.end()` to stop crashing, which this PR delivers.
… statusMessage / headers) Follow-up on the same PR — the previous commit made `http.request(...)` return a usable ClientRequest handle, but `(res) => res.statusCode` inside the response callback still returned `undefined` because IncomingMessage handles had no property dispatch. - `perry-ext-http`: export `js_http_is_incoming_message(handle)` so callers can gate property dispatch on registry membership (avoids colliding with unrelated handles whose ids happen to overlap). - `perry-stdlib/common/dispatch.rs`: add a `statusCode` / `statusMessage` / `headers` arm in `js_handle_property_dispatch`, gated on `external-http-client-pump`. Calls the existing `js_http_status_code` / `js_http_status_message` / `js_http_response_headers` externs from perry-ext-http. - `test-files/test_node_http_client_request.ts`: assert `res.statusCode` actually reflects the response. Verified against an external endpoint (`http://example.com/` → `statusCode: 200`, `statusMessage: "OK"`, `headers["content-type"]: "text/html"`).
The cargo-test in #771's CI flagged that the new `ClientRequest.setTimeout` NativeModSig entry has no manifest counterpart. Existing class-filtered http methods (on/end/write/ setHeader) collapse to the manifest entries already covering ServerResponse / IncomingMessage / HttpServer — only setTimeout was genuinely new on the http module.
CI parity on #771 surfaced \`test_node_http_client_request.ts\` as an output mismatch — the three concurrent requests fired their \"status\" prints in tokio-scheduling-dependent order, which diverged from Node's behavior run-to-run. Chain the requests instead (req2 inside req1's response callback, req3 inside req2's) so the line order is deterministic and matches \`node --experimental-strip-types\` byte-for-byte. Also dropped the per-path \`statusCode\` setter from the in-process server — it was unused (Perry and Node both default to 200) and only confused the expected output. Verified locally: Perry and Node both produce: server listening req1 status: 200 req2 status: 200 req3 status: 200 done
78937af to
a595d55
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #769 (split off from #767 case 3).
http.request(...)andhttp.get(...)(and thehttps.*variants) always returnedundefinedfrom compiled TypeScript, so the standardconst req = http.request(url, cb); req.on('error', ...); req.end()pattern crashed at.onwith "Cannot read properties of undefined". The runtime implementations existed inperry-ext-httpand the FFI symbols were declared, but noNativeModSigentries existed in the codegen dispatch table — calls fell through to the unknown-method path and got backTAG_UNDEFINED.What changed
crates/perry-codegen/src/lower_call.rs): addNativeModSigentries forhttp.request,http.get,https.request,https.getand the ClientRequest instance methodson,end,write,setHeader,setTimeout.crates/perry-hir/src/lower.rs+crates/perry-hir/src/lower_decl.rs): tagconst req = http.request(...) | http.get(...) | https.*bindings as("http", "ClientRequest")soreq.on/.end/...dispatch through the new class-filtered entries. Mirrors the existingnet.createConnection/tls.connecttagging. Important: the closure-body path inlower_decl.rshad no equivalent of the top-level arm inlower.rs, sorequest()inside a callback (the common shape:server.listen(..., () => { ... request(...) ... })) was unreachable.perry-ext-http(crates/perry-ext-http/src/lib.rs):request_common— the previous code only handled the options-object overload, and the reporter's repro useshttp.request("http://...", cb);dispatch_requestto spawn the reqwest future as a detached task on the multi-thread runtime instead ofHandle::current().block_on. Now that the dispatch actually reaches this code,block_onpanicked with "Cannot start a runtime from within a runtime" becausespawn_blocking_with_reactorruns the closure insideruntime().spawn(async {...}). Mirrors thespawn_socket_runnerpattern inperry-ext-net, including the LTOblack_boxworkaround for tokio's CONTEXT statics;js_http_is_incoming_message(handle)so perry-stdlib can gate property dispatch on registry membership.crates/perry-stdlib/src/common/dispatch.rs): add astatusCode/statusMessage/headersarm injs_handle_property_dispatch. Without this,(res) => res.statusCodeinside the response callback returnedundefined.crates/perry-stdlib/Cargo.toml+crates/perry-stdlib/src/common/async_bridge.rs+crates/perry/src/commands/compile/optimized_libs.rs): add anexternal-http-client-pumpfeature, drainjs_http_process_pendingfrom the main-thread pump, and keep the event loop alive viajs_http_has_pendingwhile a request is in-flight. Activated by the well-known flip whenevernode:http/node:httpsroute to perry-ext-http. Mirrors the existingexternal-http-server-pumpplumbing.Test
New fixture
test-files/test_node_http_client_request.tsexercises all three forms (request(url, cb),request(options, cb),get(url, cb)) against an in-processcreateServerand verifiesres.statusCodereflects the response. End-to-end verified againsthttp://example.com/:The reporter's exact
req.on/req.endrepro from #769 also runs without crashing.cargo test --release -p perry-hir -p perry-codegen -p perry-ext-http --lib— all green. Regression-checkedfetch/axios(still work) andnet.connect(options, cb)(still fails — tracked separately in #770).Test plan
test-files/test_node_http_client_request.tsruns locally on macOS arm64res.statusCode/res.statusMessage/res.headersreturn real values against an external serverfetch/axioscontinue to return200cargo test --release -p perry-hir -p perry-codegen -p perry-ext-http --libpasses