feat!: use maxConcurrentStreams as multiplexing factor #4158
Conversation
For H2 sessions pipelining is misleading and is a concept of HTTP/1.1. This makes use of `maxConcurrentStreams` parameter to be equivalent of what H2 calls multiplexing. It makes sure that clients do not have to change `pipelining` in order to achieve multiplexing over a H2 stream. Closes nodejs#4143.
| @@ -15,7 +15,7 @@ const servername = 'agent1' | |||
| const iterations = (parseInt(process.env.SAMPLES, 10) || 10) + 1 | |||
| const errorThreshold = parseInt(process.env.ERROR_THRESHOLD, 10) || 3 | |||
| const connections = parseInt(process.env.CONNECTIONS, 10) || 50 | |||
| const pipelining = parseInt(process.env.PIPELINING, 10) || 10 | |||
| const maxConcurrentStreams = parseInt(process.env.MULTIPLEXING, 10) || 10 | |||
There was a problem hiding this comment.
Either this or:
| const maxConcurrentStreams = parseInt(process.env.MULTIPLEXING, 10) || 10 | |
| const maxConcurrentStreams = parseInt(process.env.MAX_CONCURRENT_STREAMS, 10) || 10 |
Unless you would prefer re-using the same semantic across benchmarks and keep "pipelining" here.
| if (client[kHTTPContext]?.version === 'h2') { | ||
| return client[kMaxConcurrentStreams] ?? client[kHTTPContext]?.defaultPipelining ?? 100 | ||
| } |
There was a problem hiding this comment.
What remains unclear to me is under which circumstances getPipelining can be called with a client that has no kHTTPContext present.
In any case, this tries to figure out whether we're using H2 and short circuit on that decision.
The statement below acts as a fallback but I'm almost 100% sure that client[kPipelining] always wins (is never nullish) since a client is always created with on in its constructor. However, the setter for pipelining COULD make it nullish (for some reason) and only then it would fall to the context's setting.
There was a problem hiding this comment.
We are not yet preparing a new major version, so we will need to make it backwards compatible with a warning that states the change;
We can follow the path of putting maxCurrentStreams in higher order of priority compared to pipelining for h2, but still honor pipelining if maxConcurrentStreams is not passed so we keep do not introduce a breaking change (just yet)
This relates to...
Closes #4143.
Rationale
For H2 sessions pipelining is misleading and is a concept of HTTP/1.1.
This makes use of
maxConcurrentStreamsparameter to be equivalent of what H2 calls multiplexing. It makes sure that clients do not have to changepipeliningin order to achieve multiplexing over a H2 stream.Changes
Features
pipelining.Bug Fixes
Breaking Changes and Deprecations
Status
Footnotes
This makes undici (non-fetch) results way closer to "native - http2" under a single connection. It is worth noting that any benchmarks running on 2+ connections are unfair to "native" as it has a single connection open. ↩