fix(h2): make Client multiplex on h2 (#4143) by mcollina · Pull Request #5362 · nodejs/undici (original) (raw)
Adds a minimal reproduction showing that with allowH2 enabled by default
(8.0.0+), pipelining still defaults to 1, which either serializes
concurrent requests on a shared H2 session or forces Agent to open one
TCP socket per concurrent request. Both modes look like an H2 regression
for callers like the AWS SDK.
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
Before this change, Client[kPipelining] defaulted to 1 unconditionally, so getPipelining() returned 1 even after ALPN negotiated h2 — defeating h2 multiplexing on a shared session and serializing concurrent requests behind a single in-flight stream.
Leave kPipelining null when the user did not specify it, so getPipelining() falls through to kHTTPContext.defaultPipelining (1 for h1, Infinity for h2) once the connection is established. This matches what H2CClient already does at construction time.
The h1 parser/writer paths that previously read client[kPipelining] as
truthy now use ?? 1 to preserve h1 keep-alive defaults.
The Client#pipelining getter now returns the effective pipelining via getPipelining() so it reflects the protocol-aware value.
Also makes the http2-abort.js fixture defensive against the peer RST_STREAM-ing before the server responds, which is now reachable because the concurrent requests truly multiplex.
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
repro: H2 default pipelining serializes / fans out (#4143) fix: default h2 Client pipelining so multiplexing actually works (#4143)
Per the public docs, pipelining is the HTTP/1.1 RFC7230 concurrent-request
factor (default 1), and maxConcurrentStreams is the HTTP/2 multiplexing
ceiling (default 100). Conflating them — as the resume() and Client.busy
gates did — meant a single h2 Client was capped at one in-flight stream by
the h1 pipelining factor.
Introduce a protocol-aware helper getMaxConcurrent(client) that returns
kMaxConcurrentStreams when the active context is h2, and falls back to
getPipelining() otherwise. Use it in both dispatch gates.
This preserves the public semantics of Client#pipelining (still reflects
only the h1 factor, defaults to 1) and leaves h1 keep-alive paths untouched.
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
fix: default h2 Client pipelining so multiplexing actually works (#4143) fix(h2): route Client dispatch gate through maxConcurrentStreams (#4143)
pipeliningis HTTP/1.1 RFC7230 only; note that it has no effect on h2 sessions, pointing readers atmaxConcurrentStreamsinstead.maxConcurrentStreamsdocuments its three roles: the per-Client h2 dispatch ceiling, thepeerMaxConcurrentStreamsadvertised to the server, and a pre-SETTINGSdefault rather than a hard cap (the server value replaces it whenever one is sent).
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
- H2CClient: match Client.md wording on server SETTINGS replacing the
user-supplied value, surface
peerMaxConcurrentStreams, and note that H2CClient (unlike Client) aliases pipelining to maxConcurrentStreams at construction time. - Pool: add a note that with the default unlimited
connections, Pool opens a new Client (== a new socket) per concurrent dispatch, which defeats h2 multiplexing on a shared session — to benefit from it, capconnections. - Agent: same note via its per-origin Pool.
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
With h2 multiplexing actually working, the #2364 tests can finish their
t.completed before the server-side setTimeout(400) callback fires.
When after() then closes the client, the server's h2 session reads a
RST/FIN that lands as ECONNRESET — an asynchronous error landing after
the test ended, which node:test escalates to an uncaughtException.
Attach an 'error' handler on each server-side session to swallow that
post-test ECONNRESET. The streams themselves are still guarded against
ERR_HTTP2_INVALID_STREAM.
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
The session.on('error') alone wasn't enough on macOS/Windows. Also suppress 'error' on the underlying TLS socket (via secureConnection), on session.socket, and on the http2 server itself, since the post-test ECONNRESET can land on any of those depending on platform timing.
Signed-off-by: Matteo Collina hello@matteocollina.com
Closing the client inside the test body (instead of in after()) keeps
the resulting session teardown and any ECONNRESET inside the test's
lifetime, where the error handlers can swallow it and node:test no
longer sees "async activity after the test ended" on macOS/Windows.
For the 2nd variant, also wait 500ms after closing so the server-side 400ms timer fires (it no-ops on a destroyed stream) before the test function returns.
The existing after(() => client.close()) hooks are guarded against
ClientDestroyedError now that the body closes the client itself.
Signed-off-by: Matteo Collina hello@matteocollina.com
Pool's GetDispatcher picks a Client with !kNeedDrain, otherwise spawns a new one. With the default allowH2 (https + ALPN h2), the first request opens a Client and the resulting kPending > 0 on that Client made Client[kBusy] return true — so a burst of concurrent requests would spawn one Client (and one h2 session, one TCP socket, one TLS handshake) per request during the handshake, even though all of them could multiplex on a single session once h2 negotiated.
The Client now treats h2 as "may negotiate" while the URL is https and allowH2 is on, even before the HTTP context attaches. In that state:
- kBusy ignores the per-request kPending > 0 fan-out signal
- getMaxConcurrent returns maxConcurrentStreams instead of pipelining
so concurrent dispatches share the first Client. Once the context attaches, h2 keeps multiplexing; h1 contracts back to the pipelining ceiling and the queue drains accordingly. Plain http and explicit allowH2: false are untouched.
Regression test asserts a default Pool consolidates N concurrent requests onto one h2 session / one socket.
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
fix(h2): route Client dispatch gate through maxConcurrentStreams (#4143) fix(h2): make Client and Pool actually multiplex on h2 (#4143)
Re-ran benchmarks/benchmark.js (50 connections, pipelining=10) on Node 24.14.1. Relative ordering of dispatchers is essentially unchanged.
undici - dispatch is omitted because its SimpleRequest handler still
uses the legacy onConnect/onHeaders/onData/onComplete API and trips the
current assertRequestHandler validator on main; the bench files need a
separate refresh.
Signed-off-by: Matteo Collina hello@matteocollina.com
SimpleRequest in all four bench files used the legacy
onConnect/onHeaders/onData/onComplete/onError handler API and tripped
assertRequestHandler, so undici - dispatch reported Errored on every
recent run. Migrated to the current onRequestStart/onResponseStart/
onResponseData/onResponseEnd/onResponseError API and re-ran the h1 and
h2 benches on Node 24.14.1.
Updated README:
- H1 table re-captured with the working dispatch row (now leads at ~20.4k req/sec as expected).
- New H2 table added for benchmarks/benchmark-http2.js (same shape: 50 connections, pipelining depth 10).
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
The earlier 'may negotiate h2' speculation queued every concurrent
dispatch onto the first connecting Client. When ALPN turned out to be
h1 (e.g. plain https against an h1-only server), those requests then
serialized through pipelining — a ~10x throughput regression on the
https benchmark.
Drop the speculation. Client.busy and getMaxConcurrent now consider
multiplexing only when an h2 context is actually attached. Single-Client
h2 multiplexing still works (the queued requests drain in one batch
after the context attaches) and Pool({ connections: 1 }) consolidates
onto one h2 session. The cold-burst unconstrained-Pool case (no
connections cap) still fans out one socket per request during the
TLS handshake; resolving that needs a Pool-level lazy-connect
strategy and is left for a follow-up.
Test reshaped accordingly: the unconstrained-Pool assertion is replaced
by a connections: 1 test, with an inline comment pointing at the
follow-up.
Also adds benchmarks package.json scripts for the h1-over-https bench
so the regression test path is reproducible (npm run bench:https).
Signed-off-by: Matteo Collina matteo.collina@gmail.com Signed-off-by: Matteo Collina hello@matteocollina.com
Captured all three on Node 24.14.1, 50 connections, pipelining depth 10.
- HTTP/1.1: re-captured on the same setup. Numbers within run-to-run variance of the prior table.
- HTTP/1.1 over HTTPS: new table, exercising the h1-over-TLS path that the speculation revert is meant to keep fast.
- HTTP/2: re-captured.
Signed-off-by: Matteo Collina hello@matteocollina.com
fix(h2): make Client and Pool actually multiplex on h2 (#4143) fix(h2): make Client multiplex on h2 (#4143)
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
[ Show hidden characters]({{ revealButtonHref }})