feat: use provided maxConcurrentStreams as multiplexing factor

[slagiewka]

This relates to...

Closes #4143.

Rationale

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.

Changes

Features

• Allow controlling multiplexing over a stream without increasing pipelining.

Bug Fixes

Breaking Changes and Deprecations

• None, unless users defined maxConcurrentStreams themselves.

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional) ¹
• Documented
• Review ready
• In review
• Merge ready

Footnotes

1. 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. ↩

[slagiewka]

I didn't touch the H2CClient yet, but it might be addressed as well. Granted, this has lesser priority as it already defaults pipelining to be equal maxConcurrentStreams.

[slagiewka]

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.

[metcoder95]

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)

[mcollina]

Good work! Can you fix the tests?

[metcoder95]

lgtm, just minor comment

[metcoder95]

Let's just add a deprecation warning if maxConcurrentStreams is not set, and we should be good to go

[slagiewka]

When would that happen? The symbol (which name I don't like yet 🙈 ) that I'm using is only set if maxConcurrentStreams is set (line 240).

Or your suggestion is to notify everyone using allowH2 but not setting it - therefore missing the benefit of multiplexing?

I'm also not sure WHEN that deprecation warning would happen. I assume that somewhere in Client constructor. Otherwise the noise would be significant.

[metcoder95]

I was thinking to add it only when the kMaxConcurrentStreamsManual is set to false but pipeline was altered instead; so the warning states that in further major pipeline will not take the effect of maxConcurrentStreams

[slagiewka]

One comment from me: with the original change enabling 100 concurrent streams for the entire codebase might have uncovered some issues when multiplexing is in use. Tests might be indication of that.
I'm not yet versed well enough with the internals, but that's a yellow flag to me.

[metcoder95]

One comment from me: with the original change enabling 100 concurrent streams for the entire codebase might have uncovered some issues when multiplexing is in use. Tests might be indication of that. I'm not yet versed well enough with the internals, but that's a yellow flag to me.

Haven't run the benchmarks yet (might do it Today); but what's have you see?

[slagiewka]

Haven't run the benchmarks yet (might do it Today); but what's have you see

It's actually not in benchmarks but in tests:

test at test/http2.js:1247:1
✖ #2364 - Concurrent aborts (111.166097ms)
  Error [ERR_HTTP2_INVALID_STREAM]: The stream has been destroyed
      at ServerHttp2Stream.respond (node:internal/http2/core:2786:13)
      at Timeout._onTimeout (/home/runner/work/undici/undici/test/http2.js:1252:14)
      at listOnTimeout (node:internal/timers:581:17)
      at process.processTimers (node:internal/timers:519:7) {
    code: 'ERR_HTTP2_INVALID_STREAM'
  }

test at test/http2.js:1:1

Which is 100% reproducible for me when pipelining == default maxConcurrentStreams (original commit) and does not show up otherwise (current state and on main).

At first I thought that maybe there's something wrong with how "destroy" happens on a stream once request gets aborted and instead the session gets hit. But the error happens on the mock server in this test (trying to respond) - haven't looked yet into why that may happen with many streams vs a single stream.

[metcoder95]

I've been running benchmarks with different combinations and I'm finding a strong correlation between the pipelining (parallel requests send through the same session within undici) and the number of max concurrent streams available; having a negative impact on performance if both increased in the same direction (both increased and close to each other).

See following table

• maxConcurrentStreams = 100
• pipelining = 50

in contrast with

• maxConcurrentStreams = 100
• pipelining = 100

The results are consistent between executions, feel free to play with the combination, but the closer the pipelining is to maxConcurrentStreams, the worst the performance becomes.

Given the results and findings, I'll suggest to re-evaluate the change and, instead of keeping them in sync, maintain pipelining as a way to handle concurrent requests within undici while maxConcurrentStreams maintains keeping the stream management independent.

If we want to be explicit, we can add a further option property (e.g. multiplexing) that states the max number of concurrent requests while maxConcurentStreams remains applying to the same concept (although I believe pipelining can keep it with just extra documentation added).

One thing that we need to establish for sure is that pipelining cannot be greater than maxConcurrentStreams.