feat: Add H2 support

[metcoder95]

Superseeds #1014
Closes #399

Checklist - APIs

• Support for Client#Connect
• Support for Client#Dispatch
• Support for Client#Request
• Support for Client#Stream
• Support for Client#Pipeline
• Support for Client#Destroy
• Support for Client#Close
• Support for H2 settings when creating a stream at the different APIs
• Support for disabling H2 whenever ALPN says is available
• Internal stream management (max concurrent, and usage of them)
• Documentation
• Experimental warning
• Calculate content-length before triggering the request

Checklist - HTTP2

• 100 continue
• SETTINGS - Client
  • Not planned
• SETTINGS - Server (triggered by)
  • Does not needed, as is automatically applied to the session
• GOAWAY - Client
• GOAWAY - Server (triggered by)
• PING - Client
• PING - Server (triggered by)

[codecov-commenter]

Codecov Report

Patch coverage: 74.90% and project coverage change: -0.49% ⚠️

Comparison is base (ea4f257) 86.00% compared to head (1433624) 85.51%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2061      +/-   ##
==========================================
- Coverage   86.00%   85.51%   -0.49%     
==========================================
  Files          76       76              
  Lines        6637     6856     +219     
==========================================
+ Hits         5708     5863     +155     
- Misses        929      993      +64     

Files Changed	Coverage Δ
lib/api/api-request.js	98.86% <ø> (ø)
lib/core/symbols.js	100.00% <ø> (ø)
lib/core/request.js	87.01% <44.44%> (-8.99%)	⬇️
lib/client.js	93.11% <78.43%> (-3.58%)	⬇️
lib/fetch/index.js	85.20% <88.23%> (-0.13%)	⬇️
lib/api/api-connect.js	97.87% <100.00%> (+0.09%)	⬆️
lib/core/connect.js	80.00% <100.00%> (-1.02%)	⬇️
lib/core/util.js	95.78% <100.00%> (+0.02%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

[metcoder95]

Hi!
I just started the work by rebasing and adjusting the set of changes made previously with the current state of the main, this allowed me to understand in deep the changes related to that enabled the support of the H2 protocol.

I just left a couple of questions on some of the topics, as well I would like to understand more or less what other expectations are outside of the comments and just plain h2 support.
This includes UPGRADE requests with a positive response from the server, and other aspects (I'll create a checklist for readiness and keeping track of the features).

As well, there was a concern from @ronag about better use of the branch from @jasnell on QUIC, but not sure if this is really wanted, but is still something to check.

Let me know your thoughts and I'll come back to this later this week 🙂

cc: @mcollina

[ronag]

I'm not a fan of the node core http2 module. I won't block, but I will discourage its usage. I am a fan of the quic/http3 work though.

[metcoder95]

I'm not a fan of the node core http2 module. I won't block, but I will discourage its usage. I am a fan of the quic/http3 work though.

I don't have a strong opinion on this one, so I opened it again to refresh my ideas and thoughts about it 🙂

Is this still something we want to do?

Maybe would be better to target @jasnell's node http3/quic branch? I'm not too comfortable with node core http2. nodejs/node#38233

Note: Superseeded by nodejs/node#44325

[ronag]

I think the best way to implement this is to only modify connect to set a socket:like object on the client. Any further changes than that seems to me like complicating things. But maybe I'm missing something.

[metcoder95]

I think the best way to implement this is to only modify connect to set a socket:like object on the client. Any further changes than that seems to me like complicating things. But maybe I'm missing something.

Now that you mention it, I think we can point in that direction. Though, the writeH2 basically reuses the biggest portion of the normal write one with some tweaks.
The key differentiators are:

1. The parser (llhttp for H1, HTTP2ClientSession for H2), for response body and headers, as well as requests ones.
2. Session events adjustments, including some weird behaviors of one stream compared to the other

I'll try to bring support for the bare basics of the H2, and do cleanup either here or in another PR. But so far, I think we can consolidate them into a single set of adjustments to mimic the socket shape expected by undici

[metcoder95]

Update
I'm afraid already reach the most extent that can be done for H2 independently. As suggested by @ronag, I'll start working on an abstraction on top of H2 Node.js module that plays compatibility with Undici interface expectations.
I might require to also adjust some of the parts that undici already has to cope with the different scenarios, but at least String/Buffer and Streams are fully supported. For instance, the bare basics are there, good enough to start the abstraction.

Please @KhafraDev, @mcollina , if there's feedback at the way the things for the next steps, please let me know 🙂

[mkaufmaner]

Update I'm afraid already reach the most extent that can be done for H2 independently. As suggested by @ronag, I'll start working on an abstraction on top of H2 Node.js module that plays compatibility with Undici interface expectations. I might require to also adjust some of the parts that undici already has to cope with the different scenarios, but at least String/Buffer and Streams are fully supported. For instance, the bare basics are there, good enough to start the abstraction.

Please @KhafraDev, @mcollina , if there's feedback at the way the things for the next steps, please let me know 🙂

@metcoder95 Thank you for your hard work on supporting http/2. Keep it up!

I know the interfaces are completely different for undici and the node http/http2 modules but have you considered leveraging other http2 wrappers as a basis?

For example;
https://github.com/szmarczak/http2-wrapper
https://github.com/grantila/fetch-h2

[metcoder95]

Hi @mkaufmaner, thanks for the kind words! 🙇

The interface "struggles" mainly were how undici handles TCP/HTTP connections.

TL;DR It does not use node:http but writes directly against the TCP socket, handling the parsing itself (on top of llhttp).

Though, in the end, wasn't that complicated, and found a small way to do it. I'll clean up in the next few days and set the PR ready for review

[mkaufmaner]

Hi @mkaufmaner, thanks for the kind words! 🙇

The interface "struggles" mainly were how undici handles TCP/HTTP connections.

TL;DR It does not use node:http but writes directly against the TCP socket, handling the parsing itself (on top of llhttp).

Though, in the end, wasn't that complicated, and found a small way to do it. I'll clean up in the next few days and set the PR ready for review

Amazing, really looking forward to seeing the PR when it is ready for review!

I know undici doesn't use node:http (the whole reason undici is fast) but guess I haven't really comprehended the intricacies of interfacing with llhttp. I'll spend some time myself reading through undici source code to improve my comprehension.

Thanks again!

[mateus4k]

@metcoder95, do you need some help with this? I'm not an undici contributor, but I'd like to use H2 and I can try to contribute with it

[metcoder95]

@metcoder95, do you need some help with this? I'm not an undici contributor, but I'd like to use H2 and I can try to contribute with it

Hi @mateus4k, thanks for volunteering for it!
It would actually will be nice, but for this work I’ll be wrapping up the work for initial review. I would say that I won’t cover all the corner cases so quite few things will pop-up out of it 👍

[metcoder95]

I think the PR is in a reviewable state. Please let me know your thoughts 🙂

I tried to make the H2 implementation match Undici expectations of streams/sockets, but the implementation is heavily tight to H1 that making an effort and trying to do so will require several rewrites that might be better to have them done in a separate PR, or maybe not at all.

There are several other pieces of H2 support that might be worth it to chat about it to evaluate whether to add it or not (maybe even create its own issues for others to contribute).

Note: On the testing, it seems something is failing over async-hooks on 20 and 19, couldn't narrow them down, but maybe you have a better idea of what can I do to fix it 🙂

[mcollina]

Good work. This is a completely different implementation to what I would have done. I have a few questions:

1. how many concurrent requests are possible for a given Client?
2. could you add a test with a HTTPS+HTTP2 server, showing that the connection uses HTTP2?

[mcollina]

remove this

[mkaufmaner]

2. could you add a test with a HTTPS+HTTP2 server, showing that the connection uses HTTP2?

@mcollina Hopefully you wouldn't mind if we used fastify for this since writing a one off ALPN negotiator seems a little redundant.

@metcoder95 if @mcollina doesn't mind using fastify, I have taken care of this and opened a PR into your branch. See metcoder95#34

[KhafraDev]

I'm a strong -1 on adding more dependencies.

[metcoder95]

Good work. This is a completely different implementation to what I would have done. I have a >few questions:

• how many concurrent requests are possible for a given Client?
• could you add a test with a HTTPS+HTTP2 server, showing that the connection uses HTTP2?

Thanks!
Going one by one:

• Haven't made that check yet, so far is not limited and we are under the limits of the node:http2 module. Let me test that out.
  • This leads me to this comment, how do we handle stream concurrency?
• Not yet, but saw that @mkaufmaner did already some research and showed good results. Based on @KhafraDev comments, I think we can adjust it to use native node:http(2) to construct a bare basic server that does the ALPN negotiation

[mkaufmaner]

Added some thoughts about concurrency and performance.

[mkaufmaner]

I think concurrency should be handled through an LRU cache scoped to the Client class. This approach would be very similar to @szmarczak https://github.com/szmarczak/http2-wrapper library.

Public Interface

First I think the addition of http2 should expose the http2.connect options to whomever is instantiating the Client class through an http2 constructor option. Maybe even possibly omitting the createConnection option?

Performance

One key aspect of performant http2 is setting the appropriate window sizing. That being said the default http2 window sizing should be resolved at some point. See nodejs/node#38426

Until the default window sizing issue is resolved the initial window size should be set;

const session = http2.connect(client[kUrl], {
  createConnection: () => socket,
  initialWindowSize: 1024 * 1024 * 32
})

Concurrency Events

If the LRU cache for concurrency approach is taken, the session events for remoteSettings and goaway need to be handled.

Remote Settings

Once the session receives a remoteSettings settings event, the session should set aClient class kMaxConcurrentStreams limit and set the local window size of the session.

// See https://github.com/nodejs/node/issues/38426
if (session.setLocalWindowSize) {
  session.setLocalWindowSize(1024 * 1024 * 4); // 4 MB
}

Within the handler for once the session receives a remoteSettings settings event, another listener should be created for on remoteSettings as the session session.remoteSettings.maxConcurrentStreams needs to update the value for the kMaxConcurrentStreams.

Goaway

If concurrency is implemented through an LRU cache, once a goaway event is received from the remote the session should be removed from the LRU cache so it won't be reused.

[mkaufmaner]

Ah, I see, the client instances are maintained in the Pool class. Thus the remote settings and http2 connection state just needs to be bubbled up to the pool.

[metcoder95]

Thanks for all the feedback, incredibly insightful 🙇

Going one by one:

First I think the addition of http2 should expose the http2.connect options to whomever is instantiating the Client class through an http2 constructor option.

I would love to hear @mcollina, @ronag, and @KhafraDev thoughts to see which options would be ok to move to the public interface, and based on that expose the one required (at least the bare minium). It is also worth it to say that Undici already exposes configs required to manipulate the Socket connection

Maybe even possibly omitting the createConnection option?

Not so sure about this one. By the nature of Undici, the initial connection is made through a socket assuming an H1 connection. If then, the server negotiates and serves a H2, is when this feature comes into play.

One key aspect of performant http2 is setting the appropriate window sizing. That being said the default http2 window sizing should be resolved at some point. See nodejs/node#38426

Good catch, seems is already approved. I'll update it then 👍

For parallel streams
Though I agree with your points, I didn't start yet with the parallel streams supports, even tho is something that should be treated with care as you suggest.
But this requires also come with a good default, in combination with an API to update several settings accordingly for concurrency.
I would like to hear thoughts from the team, in the meanwhile I'll implement the smaller pieces already pointed out and kick-off benchmarks as well 😄

[mkaufmaner]

@metcoder95 @KhafraDev @mcollina I created some simple benchmarks against the http/2 implementation and opened a PR metcoder95#35 into the http2 branch.

The http/2 benchmark results are promising! However, there are certainly some bugs that need to be worked through as the benchmark only seems to work sporadically.

Initial Results;

│ Tests             │ Samples │          Result │ Tolerance │ Difference with slowest │
|───────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http2 - request   │       1 │  438.80 req/sec │  ± 0.00 % │                       - │
|───────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - fetch    │       1 │  486.82 req/sec │  ± 0.00 % │               + 10.94 % │
|───────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - pipeline │       1 │ 1114.42 req/sec │  ± 0.00 % │              + 153.97 % │
|───────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - stream   │       1 │ 1319.30 req/sec │  ± 0.00 % │              + 200.66 % │
|───────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - request  │       1 │ 1482.31 req/sec │  ± 0.00 % │              + 237.81 % │

[metcoder95]

Nice work @mkaufmaner 🚀
Thanks for the support on the benchmarks. I have a rough idea of what might be missing. I'll go through them later on 👍

[KhafraDev]

           compare: ===
          at:
            line: 126
            column: 5
            file: test/fetch/http2.js
            type: Test
          stack: |
            Test.<anonymous> (test/fetch/http2.js:126:5)
          source: "  t.equal(responseBody, 'hello h2!')\r
          
            \  t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody)\r
          
            ----^
          
            })\r
          
            test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t
            => {\n"
          ...
        
        # failed 1 of 8 tests
    not ok 2 - [Fetch] Should handle h2 request with body (stream) # time=47.939ms

https://github.com/nodejs/undici/actions/runs/6022709926/job/16342298970?pr=2061#step:14:13589

[metcoder95]

           compare: ===
          at:
            line: 126
            column: 5
            file: test/fetch/http2.js
            type: Test
          stack: |
            Test.<anonymous> (test/fetch/http2.js:126:5)
          source: "  t.equal(responseBody, 'hello h2!')\r
          
            \  t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody)\r
          
            ----^
          
            })\r
          
            test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t
            => {\n"
          ...
        
        # failed 1 of 8 tests
    not ok 2 - [Fetch] Should handle h2 request with body (stream) # time=47.939ms

nodejs/undici/actions/runs/6022709926/job/16342298970?pr=2061#step:14:13589

Is this still happening? I'm seeing the test green.
Although I faced it several times only when running all the tests in the band, while running the test individually it passed; even through several runs. I tried to simplify its consumption so is less "susceptible" to race conditions but stears just appear sporadically.

[KhafraDev]

a few tests are still failing, on the most recent ubuntu v20 ci run

[metcoder95]

I was checking it out and for some unknown reason this test on test/fetch/blob-uri.js seems failing:

  t.test('a normal fetch request works', async (t) => {
    const res = await fetch(objectURL)

    t.plan(3)
    t.equal(blobContents, await res.text())
    t.equal(blob.type, res.headers.get('Content-Type'))
    t.equal(`${blob.size}`, res.headers.get('Content-Length'))
  })

I've tried to narrow down the possible cause of the issue but so far couldn't fully determine the problem. While debugging the issue directed me to readAllBytes in lib/fetch/util.js.
This one uses a while loop to iterate over the blob stream until fully consumed; although in two iterations should be fully consumed given its size, it seems that something cuts the processing in the middle and does not allow it to fully finish the last iteration.

async function readAllBytes (reader) {
  const bytes = []
  let byteLength = 0

  while (true) { // Enters the loop, it gets called once
    const { done, value: chunk } = await reader.read() // This is not read at the second iteration

    if (done) { // This is never true as its processing gets cut at the end of the first iteration
      // 1. Call successSteps with bytes.
      return Buffer.concat(bytes, byteLength)
    }

    // 1. If chunk is not a Uint8Array object, call failureSteps
    //    with a TypeError and abort these steps.
    if (!isUint8Array(chunk)) {
      throw new TypeError('Received non-Uint8Array chunk')
    }

    // 2. Append the bytes represented by chunk to bytes.
    bytes.push(chunk)
    byteLength += chunk.length

    // 3. Read-loop given reader, bytes, successSteps, and failureSteps.
  }
}

I'm not sure at what extent this has to do with H2 as only the header parsing was altered 🤔

[KhafraDev]

it's a node bug, fixed by nodejs/node#48935 which hasn't landed yet

[metcoder95]

Nice, thanks! Wasn't aware of that. Then, waiting for its approval to land 👍

[mcollina]

@KhafraDev could we skip that test for now? It won't work in Node 18 for a while anyway.

[KhafraDev]

It seems to work in node 18, I guess whatever commit broke it never got back ported. I don't love the idea of skipping it completely because it's a very basic test that should work. Plus we would probably also have to disable a bunch of WPTs to get v20 to pass... is there something keeping v20.6.0 from releasing? If there is it's fine to skip them since it'd be unfair to keep this pr in review for weeks while waiting for an unrelated fix

[mcollina]

Let's wait then ;)

[metcoder95]

Perfect! SGTM 👍

[01101sam]

tested on prod, works very great 👍🏻
I've uninstalled fetch-h2 and node-fetch and replaced with this PR

[KhafraDev]

@metcoder95 can u push an empty commit so we can rerun the ci? Blob issue should be fixed now that v20.6.0 landed