http: address review feedback for packed headers - #20
Draft
anonrig wants to merge 38 commits into
Draft
Conversation
PR-URL: nodejs#65114 Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: ulofiai <[email protected]> PR-URL: nodejs#65118 Fixes: nodejs#63638 Refs: libuv/libuv#5152 Refs: libuv/libuv@e640dc9 Reviewed-By: Antoine du Hamel <[email protected]>
Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65250 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Stewart X Addison <[email protected]> Reviewed-By: Mike McCready <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Beth Griggs <[email protected]>
Use undefined as the no-error sentinel when cancelling broadcast and share consumers. This ensures that 0, an empty string, false, and null are propagated instead of being converted to clean completion. Make sync share surface cancellation reasons before handling detached consumers, and add regression coverage for async and sync consumers. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#64705 Fixes: nodejs#64704 Reviewed-By: James M Snell <[email protected]>
SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let the callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Covering every authorizer invocation, including the re-prepare that SQLite can run during sqlite3_step(), exposed a second and distinct hazard: reentering a statement that is currently being stepped is a use-after-free rather than a contract violation, since finalizing it frees the virtual machine under sqlite3_step() and re-running it resets that machine mid-execution. Any callback SQLite invokes during execution can reach it, so a user-defined function is enough. Track the statements currently being stepped and reject reentry into only those, which leaves a user-defined function free to prepare, run, and finalize its own helper statements. Signed-off-by: Trevor Burnham <[email protected]> Fixes: nodejs#63207 Assisted-by: claude:opus-5 PR-URL: nodejs#65156 Reviewed-By: Xuguang Mei <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Signed-off-by: Guilherme Araújo <[email protected]> PR-URL: nodejs#62757 Reviewed-By: Xuguang Mei <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Edy Silva <[email protected]>
Replace the async generator backing Symbol.asyncIterator with a hand-rolled iterator. The generator machinery costs several extra promise allocations and microtask hops per chunk: yield awaits the yielded value and resolves the pending request through separate promises. Buffered chunks are now delivered as an already-resolved promise, one microtask sooner than before. Thenable chunks are still awaited before delivery, requests received while a next() is outstanding are queued, and return()/throw() before the first next() complete the iterator without touching the stream. The earlier delivery is observable by code racing an abort against the first chunk. The flatMap AbortSignal test relied on such a race; it is reworked to abort deterministically while two mappers are in flight, asserting the concurrency limit, in-flight cancellation and rejection, without depending on delivery timing or timers. streams/readable-async-iterator.js sync='yes': +32.59% (***) streams/readable-async-iterator.js sync='no': +9.84% (***) Assisted-by: Claude Fable 5 Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#64447 Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Mattias Buelens <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
The highWaterMark values were passed as properties of the underlying source and sink dictionaries, where they are ignored: a queuing strategy's highWaterMark is read from the constructors' second argument. Every configuration therefore measured the identical workload at the default highWaterMark of 1, which also explains the historically high run-to-run variance of this benchmark family. Pass the strategies as the constructors' second argument and cover the default (1) alongside buffered (1024, 4096) configurations. Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65138 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Three related reductions on the per-chunk paths: Wrap user sink.write and source.pull callbacks without coercing their result into a promise. When the callback returns a non-thenable (the common synchronous case), fulfillment is guaranteed and no then() lookup is observable, so the fulfilled reaction is enqueued through a single shared resolved promise at the exact microtask position the coerced promise's reaction would have had, skipping the implicit async-wrapper promise per chunk. Thenable results go through PromiseResolve(), which matches the spec's "a promise resolved with" conversion (identity for native promises). Park pipeTo's pump on backpressure by installing a record that duck-types the writer's lazily-materialized [[readyPromise]] record and whose resolve function is the pump continuation itself. Backpressure clearing then resumes the pump directly instead of materializing a fresh promise record plus reaction per flip, and the pump no longer schedules a microtask per batch. writableStreamUpdateBackpressure publishes the new backpressure state before resolving the ready record so the pump observes the updated value. Replace queueMicrotask() on the pipeTo and tee chunk-forwarding paths with a reaction on the shared resolved promise, which enqueues the continuation at the same position without the per-call scheduling overhead. pipe-to improves by 8-14% across all benchmark configurations, with readable-read and tee also improving in spot runs. Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65138 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
The start, pull, and write non-op algorithms are all raw callbacks with an identical empty body now, so a single shared nonOpCallback replaces nonOpStart, nonOpPull, and nonOpWrite. Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65138 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Signed-off-by: greenhead <[email protected]> PR-URL: nodejs#65265 Refs: https://www.sqlite.org/session/c_changeset_abort.html Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
The pointer range test creates multiple closures from the same function literals and explicitly requests synchronous optimization. V8 can also schedule concurrent recompilation for those closures. Wait for background optimization before closing the dynamic library so compiler jobs cannot outlive the fast FFI metadata they reference. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#65300 Refs: https://github.com/nodejs/reliability/issues?q=sort%3Aupdated-desc%20%22test-ffi-fast-integer-validation%22 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]>
The quic implementation calls setWakeUp with the assumption, that it is only executed once per event loop cycle. This assumption is wrong. Only setImmediate will guarantee, that the execution is delayed to later in the event loop and happening once in the event loop. Fixes: nodejs#64035 Signed-off-by: Marten Richter <[email protected]> PR-URL: nodejs#64044 Reviewed-By: James M Snell <[email protected]>
Signed-off-by: Felix P. <[email protected]> PR-URL: nodejs#65268 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
Signed-off-by: Guilherme Araújo <[email protected]> PR-URL: nodejs#62241 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Signed-off-by: Filip Skokan <[email protected]> Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
To hopefully get to the bottom of WPT crashes that have no traces. Signed-off-by: Filip Skokan <[email protected]> Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Filip Skokan <[email protected]> Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: NIxxy25 <[email protected]> PR-URL: nodejs#65271 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aviv Keller <[email protected]>
Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64986 Reviewed-By: Antoine du Hamel <[email protected]>
Replace `Array.prototype.forEach()` with `for...of` loops across 17 tests in `test/parallel`, so each loop body reads as a plain statement rather than an arrow callback. None of the iterated values are sparse arrays, the one case where `forEach` and `for...of` genuinely differ, so both constructs visit the same elements in the same order. No callback relied on `this`, an early return, or async behaviour, and the number of assertions run in each file is unchanged. Signed-off-by: Phillip Markert <[email protected]> PR-URL: nodejs#65272 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
Signed-off-by: freida-code <[email protected]> PR-URL: nodejs#65270 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
PR-URL: nodejs#65224 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Signed-off-by: Lazizbek Ergashev <[email protected]> PR-URL: nodejs#65283 Fixes: nodejs#65280 Refs: nodejs/node-v0.x-archive#853 Refs: nodejs@3935adc Refs: nodejs#18297 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]>
the binary-upload target uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar.xz as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME). Since BINARYNAME includes the optional VARIATION when present this gets missed out int he binary-upload target, for example during a release build for Alpine/musl. This commit changes the binary-upload target to use the same variable for the tarball that is used when the file is created. Signed-off-by: Stewart X Addison <[email protected]> PR-URL: nodejs#65282 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]>
Replace regex-based header token/value checks with byte lookup tables, cache OutgoingMessage lenient-validation, and coalesce headers with small Buffer bodies into a single socket write. Scan IncomingMessage rawHeaders for Content-Length and Transfer-Encoding so optimizeEmptyRequests does not force req.headers construction. On the HTTP/2 path, skip toLowerCase for already-lowercase names, use a Set for sensitive/single-value header checks, and reserve outgoing session storage to avoid reallocs while gathering nghttp2 frames. Signed-off-by: Yagiz Nizipli <[email protected]>
Send headers, the last chunk, and the chunked terminator in a single write() when res.end() is used with Transfer-Encoding: chunked. Reuse prebuilt HTTP/1.1 status lines for default reason phrases, skip toLowerCase() on common outgoing header names, and hoist HTTP/2 header serialization off the per-call closure. Signed-off-by: Yagiz Nizipli <[email protected]>
The chunked end() fast path concatenated headers with the body and wrote the result using the body encoding. That re-encoded obs-text header values as UTF-8 and reduced corked res.end() to a single socket.write(), which broke test-http-server-non-utf8-header and test-http-response-cork. Copy headers as latin1 into the combined buffer, and accept a single write after uncork. Signed-off-by: Yagiz Nizipli <[email protected]>
JS lookup tables were slower than V8's regex for header values and for token names longer than ~10 bytes. Restore the regex path for those cases, and use the table for names of length <= 10 so Connection / Keep-Alive stay on the faster path. Signed-off-by: Yagiz Nizipli <[email protected]>
Pass NativeHttpHeaders from the parser instead of a JS string array. IncomingMessage materializes rawHeaders/headers only when read. Host/Expect/body-header checks use C++ has/get. Skip Buffer::Copy for dumped bodies via parser.setSkipBody(). Signed-off-by: Yagiz Nizipli <[email protected]>
Store header name/value bytes in a packed Buffer instead of a per-request native BaseObject or JS string array. IncomingMessage materializes rawHeaders only when read. Host/Expect/body-header checks use flag bits so the default server path never creates header strings. Dumped bodies skip Buffer::Copy via setSkipBody(). Signed-off-by: Yagiz Nizipli <[email protected]>
Drop the extra util-inl.h include, wrap lines to 80 columns, and apply clang-format so format-cpp / lint-cpp pass. Signed-off-by: Yagiz Nizipli <[email protected]>
Write packed incoming-header integers as little-endian so s390x and AIX agree with JS. Export the layout constants from C++ instead of duplicating them. Keep IncomingMessage.rawHeaders as an own property to avoid a semver-major prototype change. Queue end() callbacks before the combined write so sync socket writes still succeed. Drop the HTTP/2 toLowerCase micro-optimizations and the unused unpackHeaderList export. Co-authored-by: Yagiz Nizipli <[email protected]>
Restore the buildNgHeaderString JSDoc, wrap the 400-response assertion in mustCall, and use mustSucceed for the sync end() callback test. Co-authored-by: Yagiz Nizipli <[email protected]>
These files require internal/test/binding, which is only available when Node is started with --expose-internals. Co-authored-by: Yagiz Nizipli <[email protected]>
Exercise _hasHeader/_getHeader/_hasBodyHeaders after rawHeaders is materialized, plus empty name/value binding helpers, so the JS scan paths stay covered once the C++ buffer is dropped. Co-authored-by: Yagiz Nizipli <[email protected]>
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
17 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.
This addresses the review comments on nodejs/node#65332 (
CHANGES_REQUESTEDfrom @jasnell, plus the Copilotend()callback note). The same commits have been pushed tocursor/http-http2-perf-f24cso that PR is updated.Review fixes
memcpyof host-endianuint32_tdisagreed with JS, which always reconstructed count/flags from individual bytes as LE. That matches the Jenkins failures onrhel*-s390xand AIX.HTTPParserso JS does not keep a second copy.rawHeadersstays an own property:IncomingMessagestill constructsthis.rawHeaders = []. Packed headers install an own accessor that materializes into an own data property on first read, so this is not a prototype-getter / semver-major change.end(chunk, cb)race: the user callback is queued beforewrite_(), so a synchronoussocket.write()callback (combined chunked-end viaassignSocket()) cannot mark the message finished first and then reportERR_STREAM_ALREADY_FINISHED.toLowerCasemicro-opts removed: measured HTTP/2 header work was within noise; the duplicated skip-toLowerCasehelpers are gone.buildNgHeaderStringstill uses the hoisted processor /SafeSetcleanup._hasBodyHeaders()fallback; Title-Case, lowercase, andtoLowerCase()remain.unpackHeaderListexport removed from_http_common(it was only used by the parser binding test, which now materializes viainternalBinding).Tests
NHDRbytes) intest-http-parser.js.rawHeaders, Host / Expect / TE / body-header lazy checks, and materialization fallbacks intest-http-native-headers.js.end(chunk, cb)withassignSocket()+ custom Writable.// Flags: --expose-internalson tests thatrequire('internal/test/binding').Locally, 780/780
test-http*/test-https*/test-http2*(parallel + sequential + pummel + async-hooks) passed on a release build. ESLint on the touched JS files andcpplintonsrc/node_http_parser.ccare clean.The previous x86_64-darwin shared-library job on the original PR failed installing Cachix (
cachix.orgDNS), not compiling this change. Regular macOS CI passed.