feat(http): route the node:http client's simplest shape onto turnloop - #11091
proggeramlug wants to merge 1 commit into
Conversation
`client_turnloop::try_dispatch` drives `turnloop_http::client::Http1Connection` over `perry_ffi::turnloop_net` on the agent's own loop, and is offered every exchange in `dispatch_request_snapshot` before the reqwest path. Lane 1 takes cleartext `http://` with no body, no explicit Agent, no per-request timeout and no proxy; everything else is a named decline that falls through to reqwest unchanged, which is the coexistence rule `fetch` already uses. The JS surface does not move: `PendingHttpEvent` was already transport-agnostic, so the lane emits the same ResponseHead/ResponseChunk/ResponseEnd/TransportError the reqwest task emitted and the existing drain handles them, agent admission included. Keep-alive is deliberately not in this lane — releasing a socket before `Event::End` is what misattributes framing, and that hazard gets a change of its own. `scripts/tokio_inventory.json` keeps the reqwest edge but its `reached_when` no longer says "always", and its `blocker` is corrected: agent.rs is an admission engine above the transport, not a duplicate of reqwest's pool, and the third raw-TcpStream bypass is `client_upgrade.rs`, not `agent.createConnection`.
|
Warning Review limit reachedNext included review available in 50 seconds. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
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 |
06e010c to
6063c9d
Compare
Resolving #11091's Cargo.lock conflict by taking main's lockfile silently discarded #11067's group bump, which had been cherry-picked earlier in the same assembly: mongodb 3.9.1->3.9.0, clap 4.6.7->4.6.6, toml 1.1.6->1.1.5, http 1.5.0->1.4.1, hickory-proto 0.26.3->0.26.2, cc 1.4.6->1.4.5, uuid 1.26.1->1.26.0. Worth noting WHICH gate caught it. `lock_no_downgrade.py --vs origin/main` stayed GREEN, correctly: relative to main nothing moved backwards, the bumps simply never happened. Only `tokio_inventory.py` saw it, because it compares against its own recorded list rather than against the merge base: Cargo.lock version change: mongodb ['3.9.1'] -> ['3.9.0'] Restored by taking #11067's lockfile and re-resolving the merged manifests on top, rather than hand-merging. Verified: cargo metadata --locked rc=0 (lock satisfies every manifest) tokio_inventory.py unchanged lock_no_downgrade.py --vs main no resolved version moved backwards rustls 0.23.45, getrandom 0.4.3 both at or above their floors
|
Landed on Cherry-picked from this PR's head Nothing needed from you. Thanks. |
First lane of moving the
node:http/node:httpsCLIENT off reqwest.perry-ext-http -> reqwestis one of only four tokio edges reached ALWAYSrather than as a fallback, and six of the repo's seventeen live in this crate.
No tokio edge is removed yet —
tokio_inventory.pyis unchanged at 17/7,which is correct: reqwest still serves everything outside lane 1. Only the
annotations move.
What this does
client_turnloop.rsdrivesturnloop_http::client::Http1Connectionoverperry_ffi::turnloop_neton the agent's own loop. It is offered every exchangein
dispatch_request_snapshot, immediately beforedispatch_request— threehunks in
lib.rs, nothing else moved.PendingHttpEventwas alreadytransport-agnostic, so the lane emits the same
ResponseHead/ResponseChunk/ResponseEnd/TransportErrorand the existingdrain, agent-admission release included, is untouched.
Two behaviours fall out rather than being ported: Node's client never follows a
3xx (driving the codec directly means nothing follows anything), and
res.statusMessageis unchanged —http1::Headcarries no reason phrase, sothe canonical one stands in, exactly as reqwest did.
Decline set
Narrow and named, per the pattern
fetchestablished: a non-default agenthandle, a non-empty body, an explicit timeout,
NODE_USE_ENV_PROXY=1,TE: trailers/Connection: Upgrade/Expect: 100-continue, non-http://,!available(), and whateverclient::Request::newrefuses.The three bypass headers are declined on the same predicates those bypass
modules trigger on, so the routing cannot disagree with itself.
One decline that was not anticipated: an explicit
Hostheader.Request::headstrips a caller'shostand substitutes the URL authority(a Fetch rule); reqwest sends what the caller set. A unit test pins that the
codec really does rewrite it.
Keep-alive is deliberately out of this lane. Releasing a connection before
Event::Endis the framing-misattribution hazard, and it deserves its ownchange. The cost is invisible to JS:
req.reusedSocketandagent.sockets/freeSocketsare fed byagent.rs's facade pool, alreadydecoupled from the physical connection.
Verification
cargo check --workspace --all-targets(UI excludes): exit 0, re-run at thefinal tree state
cargo test -p perry-ext-http: 148 lib + 2 integration, 0 failedtests/turnloop_client_exchange.rs, new end-to-end binary in its own process,asserting liveness three ways —
try_dispatchreturned true,completed_total()moved, and a plainTcpListenersaw a well-formed head.Sabotage-proven: forcing the lane to decline makes it fail with the
intended message, so a green run means the lane actually ran
--filter test_gap_http_: 4/4 pass, 100% parity, three timesthe lane accepted the request and output was byte-identical to node v26.5.1
check_file_size.sh,addr_class_inventory.py,gc_runtime_root_holders.py,check_gc_env_knobs.py,check_node_version_consistency.py: OK. rustfmtclean, zero clippy and zero rustc warnings in the new files
scripts/run_lint_gates.shwas not run — it is broken at the base commit(
Install cargo-xwinextraction error, fixed separately in #11081), so theindividual gates were run instead.
Also fixed
A per-request reserved-id leak —
free_handle_idmissing fromon_closed,the #6441 exhaustion shape that ws, ext-net and http-server each already carry
an arm for.
Corrections to the roadmap this lane was scoped from
Several long-standing claims in
tokio_inventory.jsonturn out to be wrong, andthe annotations are updated:
agent.rsis not "a second connection pool layered over reqwest's own".It is a per-origin admission engine — FIFO waiter queue,
maxSockets/maxTotalSockets/maxFreeSockets, socket facades — sitting above thetransport, with its own unit tests. reqwest owns only the physical connection.
Its reqwest coupling is 9 call sites in 3 functions. It is not the blocker
the inventory describes and it barely shrinks. The per-name FIFO that the
migration plan assumed would need building already exists.
tokio::net::TcpStreambypasses" list names the wrongthird. Measured:
plain_client.rs(TE: trailers),continue_client.rs(Expect), and
client_upgrade.rs(Connection: Upgrade, node:http client never emits 'upgrade': a 101 Switching Protocols response is delivered as 'response' and the socket is lost #10468).client_connect_override.rs(agent.createConnection) reaches zerotokio — it runs on perry-ext-net's
raw_netvtable. All three real ones aredeletable, since
Event::Upgradeexists too.agent.createConnectionis still blocked, for a different reason thanrecorded.
Detached::from_fd+Loop::attachare real inturnloop, but abinding crate cannot reach them: there is zero
attach/adopt/from_fd/Detachedsurface inperry_ffi::turnloop_netorperry-runtime/src/turnloop_net/abi.rs. That lane needs a perry-ffi ABIaddition first.
test_gap_fetch_expect_continue_headeris unaffected — fetch: shorthand{ headers }drops a Headers instance before js_fetch_headers_to_json, silently sending no headers #11024 is a codegendefect (shorthand
{ headers }dropping aHeadersinstance), not transport,so its skip-list entry is not stale.
Remaining lanes
Lifecycledeadlinesperry_tls_session, needs per-request config rather than fetch's process-wideOnceLockPool, release only after EndRouteTcpStreamusersagent.createConnectionLane 2 wants
wants_step()/is_mid_message()/eof_is_clean(), which donot exist in alpha.6 — this lane expresses the progress loop without them and
bounds zero-consumed steps so a non-advancing decoder cannot spin the agent's
loop. It should be sequenced behind the turnloop bump (#11083).