fix(ext/node): enable HTTP parser consume fast path#33354
Merged
Conversation
nathanwhit
commented
Apr 22, 2026
| // `&mut PinScope` via `Deref` before the cast so the ExecuteContext | ||
| // sees a real PinScope pointer. | ||
| let pin_scope: &mut v8::PinScope = scope; | ||
| let scope_ptr = pin_scope as *mut v8::PinScope as *mut (); |
Member
Author
There was a problem hiding this comment.
feels kinda sus but we need to smuggle a handlescope across the callback
nathanwhit
force-pushed
the
http-parser-consume
branch
from
April 22, 2026 01:02
533c81f to
402baa9
Compare
nathanwhit
changed the title
…lasses Several bugs blocked the parser.consume path for anything other than bare TCPWrap: - try_unwrap_cppgc_object returns None for any subclass of LibUvStreamWrap (TCPWrap, TLSWrap, PipeWrap), so the consume fast path silently no-op'd. Use try_unwrap_cppgc_base_object to match via the base class. - The C callback invoked kOnExecute with the raw nread=0 sentinel (EAGAIN/WouldBlock via libuv's alloc+nread=0 idiom), firing a spurious v8 scope + function-call round trip per request. Match Node's node_http_parser.cc:OnStreamRead — skip empty reads. - Parse errors on the consume fast path invoked kOnExecute with a raw -1. The JS .execute() wrapper (http_parser.ts) converts negative returns into an Error object with code/reason/bytesParsed before onParserExecuteCommon runs its `ret instanceof Error` branch. The consume path bypassed the wrapper, so protocol errors (chunked-smuggling etc.) silently wedged the connection. Build the same Error shape in Rust. - Casting `&mut ContextScope<HandleScope>` directly to `*mut PinScope` is UB — ContextScope has a different layout. Coerce via Deref first so ExecuteContext sees a real PinScope pointer. - On the consume path the read interceptor borrows buf.base during its callback but doesn't take ownership, so the caller must free it — we weren't, leaking 64KB per read once interceptors were actually exercised.
…for upgrade handler
Two fixes on top of the parser.consume enabling:
1. When the consume interceptor sees a terminal read (nread<0,
e.g. ECONNRESET on client abort), don't hand it to the parser.
Match Node's PassReadErrorToPreviousListener: fall through to
the normal JS read path so socket.on('error')/'end' listeners
— which the HTTP server relies on to fire 'aborted' events
and close connections — actually fire.
2. On HTTP upgrade / CONNECT, onParserExecuteCommon computes
bodyHead via d.slice(bytesParsed), where d is the raw read
buffer. In the consume path the binding hands d=undefined to
JS (there is no JS-level buffer), and the slice threw a
TypeError that silently killed the callback, leaving the
server's 'connect' / 'upgrade' event un-fired. The Rust side
now passes the current read buffer as a Uint8Array second
argument to kOnExecute when upgrade is signaled; the JS side
normalizes it to a Buffer (matches the non-consume shape)
before calling onParserExecuteCommon.
Repro: node_compat test-http-aborted.js / test-http-connect.js
/ test-http-connect-req-res.js all hung before; now pass.
nathanwhit
force-pushed
the
http-parser-consume
branch
from
April 22, 2026 02:03
402baa9 to
a2103f6
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
2 participants
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.
In our llhttp wiring we had a fast path to avoid the JS readable stream and let the parser read straight from the underlying uv stream.
Several bugs blocked the parser.consume path for anything other than bare TCPWrap:
try_unwrap_cppgc_object returns None for any subclass of LibUvStreamWrap (TCPWrap, TLSWrap, PipeWrap), so the consume fast path silently no-op'd. Use try_unwrap_cppgc_base_object to match via the base class.
The C callback invoked kOnExecute with the raw nread=0 sentinel (EAGAIN/WouldBlock via libuv's alloc+nread=0 idiom), firing a spurious v8 scope + function-call round trip per request. Match Node's node_http_parser.cc:OnStreamRead — skip empty reads.
Parse errors on the consume fast path invoked kOnExecute with a raw -1. The JS .execute() wrapper (http_parser.ts) converts negative returns into an Error object with code/reason/bytesParsed before onParserExecuteCommon runs its
ret instanceof Errorbranch. The consume path bypassed the wrapper, so protocol errors (chunked-smuggling etc.) silently wedged the connection. Build the same Error shape in Rust.Casting
&mut ContextScope<HandleScope>directly to*mut PinScopeis UB — ContextScope has a different layout. Coerce via Deref first so ExecuteContext sees a real PinScope pointer.On the consume path the read interceptor borrows buf.base during its callback but doesn't take ownership, so the caller must free it — we weren't, leaking 64KB per read once interceptors were actually exercised.
Increases throughput by about 6% on hello world
Disclosure: used claude