http: add native single-shot response builder#64393
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Serialize the common HTTP/1.1 header block (and optional body) in C++ so _storeHeader and ServerResponse.end avoid repeated JS string concatenation and multi-write path overhead. The public API and wire format are unchanged; complex cases fall back to the existing JS path. On benchmark/http/simple.js (bytes/4/1, 50 connections, 5s) this raised throughput from ~80k to ~118-126k req/s (~+48% to +58%) on the same machine. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
anonrig
force-pushed
the
http-native-single-shot-response
branch
from
6e25748 to
8a0057a
Compare
Revert incidental ConnectionsList reformatting in node_http_parser.cc, document the buildHttpMessage flag and out-param slots in JS, and avoid Buffer.concat when pairing headers with a body by queueing a separate latin1 header write instead. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
Hot writeHead()+end() now flushes via tryFastFlushEnd instead of the full write_()/extra empty write path. Server-side native headers stay as Buffers (no latin1 string copy) until write time; the C++ builder writes into a single malloc transferred to Buffer (no std::string + Copy). Reuse a scratch flat-header array and a prebuilt 200 status line. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
lgtm
This needs a CITGM run and some validation it doesn't break express, fastify, koa, etc before shipping.
mcollina
added
the
notable-change
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
Also a benchmark CI run would be interesting. |
mcollina
added
the
baking-for-lts
github-actions
Bot
removed
the
request-ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64393 +/- ##
==========================================
- Coverage 90.25% 90.08% -0.17%
==========================================
Files 741 741
Lines 241207 242680 +1473
Branches 45430 45864 +434
==========================================
+ Hits 217698 218623 +925
- Misses 15084 15586 +502
- Partials 8425 8471 +46
🚀 New features to boost your workflow:
|
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
I think this needs a few regression tests before landing. The risky area is not just performance; the native path must preserve the exact HTTP/1 framing behavior of the existing JS path.
Main cases I’d like covered:
res.end()with chunked framing but no body must still emit the terminating0\r\n\r\n.strictContentLengthmust still throw on the singleres.end(chunk)fast path.Trailermust keep the response chunked and must not be rewritten toContent-Length.Transfer-Encoding: chunked;...must still be recognized as chunked framing.- Large
Content-Lengthbookkeeping must not be truncated through native out-params.
Implementation note: native buffer bounds checks should avoid off + n <= est because that can overflow before the comparison; prefer subtraction-style checks like off <= est && n <= est - off or checked arithmetic.
Suggested regression file: test/parallel/test-http-outgoing-native-builder-regressions.js
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');
const net = require('net');
function rawRequest(handler, callback) {
const server = http.createServer(common.mustCall(handler));
server.listen(0, common.mustCall(() => {
const socket = net.connect(server.address().port, common.mustCall(() => {
socket.end('GET / HTTP/1.1\r\n' +
'Host: localhost\r\n' +
'Connection: close\r\n' +
'\r\n');
}));
socket.setEncoding('latin1');
let raw = '';
socket.on('data', (chunk) => raw += chunk);
socket.on('end', common.mustCall(() => {
server.close(common.mustCall(() => callback(raw)));
}));
}));
}
// Empty chunked responses must still terminate the chunked body.
rawRequest((req, res) => {
res.setHeader('Transfer-Encoding', 'chunked');
res.end();
}, common.mustCall((raw) => {
assert.match(raw, /\r\nTransfer-Encoding: chunked\r\n/i);
assert.match(raw, /\r\n0\r\n\r\n$/);
}));
// Advertising trailers requires chunked framing; do not rewrite to Content-Length.
rawRequest((req, res) => {
res.setHeader('Trailer', 'X-Checksum');
res.end('ok');
}, common.mustCall((raw) => {
assert.match(raw, /\r\nTrailer: X-Checksum\r\n/i);
assert.match(raw, /\r\nTransfer-Encoding: chunked\r\n/i);
assert.doesNotMatch(raw, /\r\nContent-Length:/i);
assert.match(raw, /\r\n2\r\nok\r\n0\r\n\r\n$/);
}));
// Transfer-Encoding parameters are valid transfer-coding parameters and must
// still be recognized as chunked framing.
rawRequest((req, res) => {
res.setHeader('Transfer-Encoding', 'chunked;foo=bar');
res.end('ok');
}, common.mustCall((raw) => {
assert.match(raw, /\r\nTransfer-Encoding: chunked;foo=bar\r\n/i);
assert.doesNotMatch(raw, /\r\nContent-Length:/i);
assert.match(raw, /\r\n2\r\nok\r\n0\r\n\r\n$/);
}));
// strictContentLength must be enforced for the single res.end(chunk) path too.
{
const server = http.createServer(common.mustCall((req, res) => {
res.strictContentLength = true;
res.setHeader('Content-Length', '4');
assert.throws(() => res.end('hello'), {
code: 'ERR_HTTP_CONTENT_LENGTH_MISMATCH'
});
res.destroy();
server.close(common.mustCall());
}));
server.listen(0, common.mustCall(() => {
const socket = net.connect(server.address().port, common.mustCall(() => {
socket.end('GET / HTTP/1.1\r\n' +
'Host: localhost\r\n' +
'Connection: close\r\n' +
'\r\n');
}));
socket.resume();
socket.on('close', common.mustCall());
}));
}
// Do not truncate Content-Length bookkeeping through native out-params.
{
const server = http.createServer(common.mustCall((req, res) => {
res.setHeader('Content-Length', '4294967296');
res.writeHead(200);
assert.strictEqual(res._contentLength, 4294967296);
res.destroy();
server.close(common.mustCall());
}));
server.listen(0, common.mustCall(() => {
const socket = net.connect(server.address().port, common.mustCall(() => {
socket.end('GET / HTTP/1.1\r\n' +
'Host: localhost\r\n' +
'Connection: close\r\n' +
'\r\n');
}));
socket.resume();
socket.on('close', common.mustCall());
}));
}Run with:
tools/test.py test/parallel/test-http-outgoing-native-builder-regressions.js
|
+1 on the test concerns, notably we do already have a bunch of existing tests around all this stuff, so it would be really interesting to know which of those are now using the fast path or still on the old path after this change, because either:
A setup covering a bunch of the important cases structured such that they can run in fast path or slow path mode, to verify both paths against the same set of tests would be fantastic, if that's doable (or at least for some subset of the test cases). My biggest concern here is that these paths will diverge and create very confusing issues, so any way to bind them closer together would be great. |
Address review feedback: keep Trailer/TE-with-params on the chunked path, emit the empty chunked terminator from the single-shot fast path, enforce strictContentLength on res.end(chunk), avoid truncating large Content-Length values via Float64 out-params, and use overflow-safe buffer bounds checks. Add dual-path regression coverage. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
Keep writeHead()/_storeHeader() on the legacy JS path: the C++ builder was a net regression for small-header (simple.js) and many-header (headers.js) workloads. Reserve buildHttpMessage for tryFastEnd() single-shot end(body) with bodies up to 16KiB, and never concatenate larger payloads into a combined headers+body write. Also type buildHttpMessage on the http_parser binding. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
Avoid cork/uncork on single-write flushes, combine small Buffer bodies with the header into one write for the simple.js type=buffer path, and use corked dual writes (not uncorked queued writes) for large string bodies so end-vs-write-end stays competitive without O(n) copies. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
|
CI is red |
Applying your suggestion caused a regression in performance. Trying to fix. |
Cache complete wire-format responses for repeated writeHead+end, write small responses through the stream handle, and cut keep-alive overhead with a permanent socket close hook and deferred idle timeouts. Also fix binary Content-Disposition framing under the latin1 flush path and several finish/request edge cases. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
Review feedback status (@mcollina CHANGES_REQUESTED)The items from the CHANGES_REQUESTED review look fully addressed on the current branch. Asking for a re-review when convenient. Where it landedCommit: Introduces both:
Requested case → coverage mapping
Dual-path noteThe regression file re-execs itself with @mcollina — could you take another look when you have a moment? Thanks! |
Re: dual-path / existing suite coverage (@pimterry)Good question on which tests hit the fast path vs the slow path after this change. What takes the fast path
Standalone Dual-path regression coverage
That gives parity checks for the important framing edge cases (empty chunked terminator, Trailer, TE with params, Existing suiteThe broader
So we are not relying on only one path being covered: the dedicated dual-run regressions pin the risky framing cases, and the existing suite continues to hit both shapes in normal conditions. Happy to extend the dual-path harness to more cases if there are specific suites you want mirrored. |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
lgtm
a final benchmark-ci and citgm run would be useful
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
Took another look. There are a few things to fix.
I asked the AI write an analysis of a few points.
1. implicit UTF-8 bodies are emitted as Latin-1
prepareFastBody() correctly computes the UTF-8 byte length (lib/_http_outgoing.js:1689-1708), but the string-header branches concatenate the original string body and always pass the result to writeResponseDirect() as latin1 (lib/_http_outgoing.js:1876-1921). The two branches at lines 1891-1897 are identical, including the branch specifically reached for non-ASCII UTF-8.
A normal implicit response such as:
res.end('€');keeps _header as a string, advertises Content-Length: 3, and writes the body as the single Latin-1 byte 0xac. The chunked variant similarly emits a chunk size of 3 followed by one byte. On keep-alive connections this can desynchronize all subsequent responses, not merely corrupt one character.
Please add raw-wire tests for non-ASCII res.end(string) with both auto Content-Length and explicit chunked framing. The body must equal Buffer.from(string, 'utf8') byte-for-byte.
2. Caching combined responses by mutable Buffer identity leaks stale bodies
getCombinedResponse() caches the complete wire Buffer in a Map keyed only by body identity (lib/_http_outgoing.js:1777-1830). The Buffer's contents are copied into out, but a later lookup returns that snapshot without checking whether the source Buffer changed. The cache is reused by the small Buffer-body paths at lines 1967-1969 and 1999-2002.
This produces stale cross-response data and can disclose a previous request's payload:
- Set
res.sendDate = falseand use the same cached header block, for examplewriteHead(200, { 'Content-Length': '3' }). - Send a shared Buffer containing
one. - Mutate that same Buffer to contain
two. - Send it on the next response.
The second response reuses the cached combined Buffer and sends one. Mutable ArrayBufferViews must not be used as value-cache keys; either do not cache them or cache only immutable string bodies.
3. the global header cache bypasses strict validation
tryFastStoreHeaderObject() computes lenient, but no validation mode is included in any cache key (lib/_http_outgoing.js:609-681). A hit returns at lines 684-700, before validateHeaderName() and validateHeaderValue() at lines 718-719.
Consequently, a relaxed server can seed a cached header containing a control character, and a strict server using the same writeHead() object shape can emit that cached header instead of throwing ERR_INVALID_CHAR.
A deterministic regression test can disable Date, first call:
res.writeHead(200, { 'X-Test': 'a\x01b' });on a server configured with httpValidation: 'relaxed', and then repeat it on a strict server in the same process. The strict call must throw. Cache hits must never skip validation unless the cache key includes every validation input and the cached entry is known to have been validated under the same policy.
4. High: compact header-cache keys omit state that changes the wire output
The one-header key at lib/_http_outgoing.js:645-652 omits _removedConnection, _removedContLen, _removedTE, and _maxRequestsPerSocket. The two-header key at lines 634-644 also omits _maxRequestsPerSocket. A hit then overwrites the current response's state with the cached entry at lines 689-698.
For example, seed the one-header cache with { 'X-Test': 'same' }, then on an otherwise identical response call:
res.removeHeader('Transfer-Encoding');
res.writeHead(200, { 'X-Test': 'same' });
res.end('ok');The second response can replay the first response's chunked header and reset _removedTE to false, ignoring the explicit removal. Servers with different maxRequestsPerSocket values can likewise share a cached Keep-Alive: ... max=... line.
Please remove the compact keys or include every input used by header generation and every replayed side effect. Add parity tests for all three removeHeader() framing flags and for servers with different maxRequestsPerSocket values.
5. direct handle-write failures are converted into successful finish
writeResponseDirect() passes a callback to writeGeneric(), but onDirectWrite() ignores its error argument and unconditionally calls onFinish() (lib/_http_outgoing.js:1742-1770). In lib/internal/stream_base_commons.js, both synchronous dispatch errors and negative asynchronous completion statuses are delivered to that callback. Because a callback is present, the asynchronous error path does not destroy the stream itself.
An EPIPE/ECONNRESET can therefore run the response's finish event and end() callback as if the write succeeded while leaving the socket lifecycle inconsistent. The direct path must propagate the error through the same Writable/socket machinery as socket.write(), including destruction and callback/error ordering; it cannot discard the callback argument.
6. High: direct writes can overtake data already queued on the socket
The fast-path guard checks the response's own queue and cork state, but not socket.writableLength or socket.writableCorked (lib/_http_outgoing.js:1837-1849). writeResponseDirect() then bypasses stream.Writable.write() and writes directly to the libuv handle.
A deterministic case is:
res.socket.cork();
res.writeContinue();
res.end('ok');The 100 Continue response remains in the socket's Writable queue while the final response is sent directly to the handle. The client can observe 200 before 100, and the fast return also skips the normal end() uncork block. The direct path must fall back whenever the socket is corked or has queued/in-flight Writable data.
7. request dispatch bypasses EventEmitter semantics and primordials
parserOnIncoming() reads server._events.request and invokes handler.call(...) directly when there is one listener (lib/_http_server.js:1448-1459). This bypasses an overridden server.emit, domain-aware EventEmitter.prototype.emit, and EventEmitter's rejection handling. It also invokes a user-controlled .call property rather than using a primordial.
A valid listener can demonstrate the immediate compatibility break:
function listener(req, res) {
res.end('ok');
}
listener.call = null;
http.createServer(listener);EventEmitter dispatch invokes this callable normally via ReflectApply; the new path throws because handler.call is null. Request delivery should continue through server.emit('request', req, res).
| #include "stream_base-inl.h" | ||
| #include "v8.h" | ||
|
|
||
| #include <cstdio> // snprintf |
There was a problem hiding this comment.
I think the changes to this file are not used anymore, am I wrong?
| // args[7] maxRequests int32 max requests per socket (0 = omit) | ||
| // args[8] knownLength int32 content-length if already known, else -1 | ||
| // args[9] out Float64Array(kOutCount) optional result flags | ||
| void BuildHttpMessage(const FunctionCallbackInfo<Value>& args) { |
There was a problem hiding this comment.
This function and the code around it is a large part of the diff and PR description, but I don't think anything calls it here any more, none of this is used. The last commit replaced the reference to it with:
// The C++ buildHttpMessage path was slower for small messages
That's especially bad because the legacy/fast-path test coverage switch works by stubbing this out, so currently that's not actually doing anything at all 😬
5 participants
Summary
Add a C++
buildHttpMessagehelper used by the HTTP/1.1 outgoing path so the common cases avoid repeated JavaScript string concatenation and multi-write overhead:_storeHeader: builds the status/request line + header block natively (with JS fallback for special cases such as content-disposition latin1 / unique cookie joining).ServerResponse.end()fast paths:writeHead():tryFastFlushEndflushes the prebuilt header + body without the fullwrite_()/ extra empty-write path (this is thebenchmark/http/simple.jscase)._headercompatibility.Public API and on-the-wire format are unchanged.
Benchmarks
Machine-local
out/Releaseon the same host. Baseline measured before this change.benchmark/http/simple.js(primary)Other
simple.jsvariants (same tip):chunkedEnc=1(TE: chunked)len=1024content-lengthOther microbenches
end-vs-write-endmethod=endasc/64 c=50 duration=5end-vs-write-endmethod=writeasc/64 c=50 duration=5res.end('ok')only (50 clients, 3s)Review feedback (document flags, no incidental C++ reformat, no
Buffer.concat) is included; the common ASCII path still shows the large win vs baseline.Tests
Full
test/parallel/test-http*suite: 751/751 passed (pre-push on an earlier tip); targeted suite re-run after the latest speed-ups.Test plan
tools/test.py test/parallel/test-http* --mode=release(751 passed)benchmark/http/simple.jsbefore/after (and after review + further speed-ups)