undici icon indicating copy to clipboard operation
undici copied to clipboard

feat: add http2

Open Nytelife26 opened this issue 3 years ago • 42 comments

This relates to...

  • #547

Rationale

HTTP/2 support is one of the few things currently missing from Undici that sets it behind native Node http.

Changes

This pull request is a work in progress. There are no changes just yet. The plan, however, as discussed in the original issue, is to implement HTTP/2 support around a Dispatcher instance for Pool, Client and Agent.

Implementation can work in one of the following ways (see RFC7540 and RFC8740):

  1. Prior knowledge
  • Servers using HTTP/2 can advertise the capability via methods like ALT-SVC
  • Under RFC7540 Section 3.5, the HTTP/2 connection preface must be sent
  • All future frames may immediately use the HTTP/2
  1. HTTP/1.1 Initialization (servers where HTTP/2 support status is unknown)
  • Without prior knowledge, the client should start an HTTP/1.1 connection using the Upgrade header (h2c for http URI schema, h2 for TLS) and HTTP2-Settings header
  • If the response is a HTTP 101 Switching Protocols request, the request will be accompanied by a response to prior requests and the connection can upgrade to HTTP/2. If the response is normal, exempli gratia, a HTTP 200 OK, the client must not switch to HTTP/2.

As for HTTPS, TLS negotiation must occur prior to any upgrade to HTTP/2.

Features

HTTP/2 variants of Pool, Client and Agent. Alternatively, an option in the aforementioned classes that causes them to attempt a HTTP/2 upgrade.

Bug Fixes

N/A.

Breaking Changes and Deprecations

N/A.

Status

KEY: S = Skipped, x = complete

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

Nytelife26 avatar Apr 09 '21 23:04 Nytelife26

@mcollina as noted in the "Features" section of this pull request, we have two options.

"HTTP/2 variants of Pool, Client and Agent. Alternatively, an option in the aforementioned classes that causes them to attempt a HTTP/2 upgrade."

We have only discussed the prior, so I'd like to take this opportunity to ask what the desired approach is before I begin.

Nytelife26 avatar Apr 09 '21 23:04 Nytelife26

Codecov Report

Merging #720 (f5427d6) into main (dac0517) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #720   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1854      1854           
=========================================
  Hits          1854      1854           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dac0517...f5427d6. Read the comment docs.

codecov-io avatar Apr 09 '21 23:04 codecov-io

I don't think you need to develop a new Agent or Pool. What we need is either modify existing Client to support both or create a new Http2Client, rename Client to HttpClient and then create another Client that somehow switches between the two as needed.

ronag avatar Apr 10 '21 08:04 ronag

I would just focus on creating a Http2Client right now.

The hardest part is figuring out is the upgrade to HTTP/2

mcollina avatar Apr 10 '21 09:04 mcollina

@mcollina This is pretty easy. All we need to do is just create a TLS connection and check the ALPN protocol. Something like:

if (socket.alpnProtocol === 'h2') {
    const session = http2.connect(origin, {
        createConnection: () => socket
    });
    ...
}

const client = new HttpClient(socket);
...

szmarczak avatar Apr 10 '21 12:04 szmarczak

All we need to do is just create a TLS connection and check the ALPN protocol.

That depends - the protocol is different depending on whether the desired connection is HTTPS or HTTP. As I mentioned, if TLS is present, you must use ALPN first. If not, you have to follow the standard HTTP Upgrade procedure.

Nytelife26 avatar Apr 10 '21 12:04 Nytelife26

I think http2 should always be https? I don’t think we need to support the non TLS case for http2.

ronag avatar Apr 10 '21 12:04 ronag

I think http2 should always be https? I don’t think we need to support the non TLS case for http2.

That doesn't sit right with me. We shouldn't just ignore usages that are part of the specification itself for our own convenience, especially when it'll likely result in someone filing an issue later on.

Nytelife26 avatar Apr 10 '21 12:04 Nytelife26

I think http2 should always be https? I don’t think we need to support the non TLS case for http2.

It's needed and used within microservices networks.

mcollina avatar Apr 10 '21 13:04 mcollina

We shouldn't just ignore usages that are part of the specification itself for our own convenience

undici already does.

I think http2 should always be https? I don’t think we need to support the non TLS case for http2.

I thought the same - browsers don't [support h2c].

It's needed and used within microservices networks.

I believe there are better solutions in these cases. Anyway they can just use the native http2.connect instead.

szmarczak avatar Apr 10 '21 16:04 szmarczak

It's needed and used within microservices networks.

Thank you.

undici already does.

That's not quite the same as dropping an entire protocol. Although, I do trust and hope there are technical reasons for not following those parts too, beyond merely convenience of implementation.

I thought the same - browsers don't [support h2c].

Not yet. Most of the point of HTTP/2 was for browsers so It most likely will.

I believe there are better solutions in these cases. Anyway they can just use the native http2.connect instead.

You could also argue that you could use the native http instead of undici altogether. But the point of undici is that it is faster. So yes, they can use an inferior solution, but I see no reason to not do our best just because other solutions already exist.

Nytelife26 avatar Apr 10 '21 23:04 Nytelife26

Most of the point of HTTP/2 was for browsers so It most likely will.

I don't think h2c is coming anytime soon. Even CloudFlare doesn't support it.

you could use the native http instead of undici altogether

That's not quite the same as undici is a complete rewrite, it doesn't use the http module internally. The point is that internally you have to use http2.connect(...) anyway. The API is very friendly so people should use it directly whenever the can.

I'd prefer if undici targeted HTTP/2 with TLS first, but if anybody wants to go after h2c as well - I don't mind.

szmarczak avatar Apr 11 '21 20:04 szmarczak

FYI: Google Cloud Run supports h2c https://cloud.google.com/run/docs/configuring/http2.

My take is that h2c support might be really handy.

mcollina avatar Apr 12 '21 13:04 mcollina

h2c is super useful for debugging tricky protocol issues using Wireshark, etc.

kanongil avatar Apr 13 '21 14:04 kanongil

Summary

"What we need is either modify existing Client to support both or create a new Http2Client, rename Client to HttpClient and then create another Client that somehow switches between the two as needed."

Decision: To start with, I will be implementing the methods on a new Http2Client. It is likely that I will then merge those under an option into Client, given that a client with switching may be hard to maintain and create unnecessary overhead, depending on how complex the Http2Client will be.

The hardest part is figuring out is the upgrade to HTTP/2

Decision:

  • For TLS connections, the TLS side must be handled first. Then, we can follow standard procedure.
  • The client will either initialize or use the existing HTTP/1.1 connection and send a request with the HTTP Upgrade (value of h2 for HTTPS, h2c for HTTP) and HTTP2-Settings headers.
    • Implementation on capability advertisement via things like ALT-SVC is currently unclear.
  • If the response is a HTTP 101, check for responses to prior requests and begin using HTTP/2. Else, do not switch to HTTP/2.

I'd prefer if undici targeted HTTP/2 with TLS first, but if anybody wants to go after h2c as well - I don't mind.

Decision: As aforementioned, TLS support is part of the natural protocol of any HTTP/2 upgrade. It is an extension, not a separate method, and therefore both can be achieved simultaneously.

Nytelife26 avatar Apr 14 '21 12:04 Nytelife26

Does anyone here know about polyfill support for base64url and whether it would be possible to implement it here? The HTTP/2 specification requires that the HTTP2-Settings header be encoded in base64url (without padding), however, it seems to be a recent feature and I am uncertain of how it would affect compatibility if we were to use it here.

Nytelife26 avatar Apr 15 '21 00:04 Nytelife26

Just use that and we'll polyfill later.

Anyway, @panva do you know what's the best polyfill for that?

mcollina avatar Apr 15 '21 06:04 mcollina

@mcollina you can take inspiration from https://github.com/panva/jose/blob/main/src/runtime/node/base64url.ts

panva avatar Apr 15 '21 06:04 panva

What's the situation regarding client.upgrade? It's somewhat vital for this to work, however it doesn't seem to be implemented on Client or Dispatcher, despite what the documentation says.

image

Nytelife26 avatar Apr 24 '21 23:04 Nytelife26

The api methods are assigned to the prototypes in index.js. See, https://github.com/nodejs/undici/blob/main/index.js#L15.

ronag avatar Apr 24 '21 23:04 ronag

Interesting, thank you, that didn't display in GitHub search weirdly. Peculiar approach, but it makes life easier actually since I can avoid a circular dependency between the client and its http2 variant when they merge.

Nytelife26 avatar Apr 24 '21 23:04 Nytelife26

Can we safely implement a wider Client.upgrade return context? It only provides the headers and socket, but I need the full response body too.

Nytelife26 avatar Apr 25 '21 22:04 Nytelife26

Can we safely implement a wider Client.upgrade return context? It only provides the headers and socket, but I need the full response body too.

cc: @ronag @mcollina

See my above comment when you have the time, please and thank you in advance.

Nytelife26 avatar Apr 27 '21 18:04 Nytelife26

Can we safely implement a wider Client.upgrade return context? It only provides the headers and socket, but I need the full response body too.

No idea. As far as I know there is no body after upgrade so I'm not sure what you mean here...

ronag avatar Apr 27 '21 18:04 ronag

As far as I know there is no body after upgrade

According to the HTTP/2 specification, the server may bundle the response frames to any previous requests along with its upgrade acknowledgement. For that reason, I need to be able to access the response body in order to check for those.

Nytelife26 avatar Apr 27 '21 21:04 Nytelife26

Answers, anyone?

Nytelife26 avatar May 07 '21 23:05 Nytelife26

Answers, anyone?

I don’t have a better answer. As far as I know there is no body with an upgrade so your question doesn’t make sense to me. Sorry. Maybe I’m missing something...

ronag avatar May 08 '21 04:05 ronag

Right after sending the 101 status code, the server can begin speaking the new protocol, performing any additional protocol-specific handshakes as necessary. Effectively, the connection becomes a two-way pipe as soon as the upgraded response is complete, and the request that initiated the upgrade can be completed over the new protocol.

ronag avatar May 08 '21 06:05 ronag

Answers, anyone?

I didn't thought it was a question, more of a statement. I'd recommend to try and see if it's possible.

mcollina avatar May 08 '21 08:05 mcollina

I can not continue to work on this, unfortunately. I have pushed my most recent work and hopefully that in combination with this discussion will serve useful to anyone willing to pick up. I apologise and wish you all well, but I no longer have the time, as I have packages to maintain and college is a heavy weight.

Nytelife26 avatar May 19 '21 22:05 Nytelife26