Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: HTTP/2 #866

Closed
wants to merge 7 commits into from
Closed

feat: HTTP/2 #866

wants to merge 7 commits into from

Conversation

metcoder95
Copy link
Member

Inherited from #720

This relates to...
HTTP/2-3 #399

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):

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
    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

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

@metcoder95 metcoder95 changed the title Feat/http2 feat: HTTP/2 Jul 6, 2021
@metcoder95 metcoder95 mentioned this pull request Jul 6, 2021
7 tasks
@Nytelife26
Copy link
Contributor

Good luck :)

@metcoder95 metcoder95 marked this pull request as draft July 8, 2021 20:26
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2021

Codecov Report

Merging #866 (5330889) into main (d31de8c) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #866      +/-   ##
===========================================
- Coverage   100.00%   99.95%   -0.05%     
===========================================
  Files           26       26              
  Lines         2105     2108       +3     
===========================================
+ Hits          2105     2107       +2     
- Misses           0        1       +1     
Impacted Files Coverage Δ
lib/client.js 99.87% <75.00%> (-0.13%) ⬇️

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 d31de8c...5330889. Read the comment docs.

@metcoder95
Copy link
Member Author

Might sound naive but does anybody know an easy way to test an upgrade from http to http2. As the last one is executed to run under the security layer is not so easy to trigger for h2c upgrades. Otherwise, I'll need to create a small server for it (just looking for some already made solution before making a handmade one)

cc: @ronag @mcollina

@mcollina
Copy link
Member

I don't have a clue... maybe @jasnell do.

@szmarczak
Copy link
Member

@metcoder95 What security layer are you talking about?

@metcoder95
Copy link
Member Author

@szmarczak I was referring that trying to visit an HTTP2 website forces you to go under TLS, but I'm trying to test the upgrade from HTTP/1.1 to HTTP/2 with h2c upgrade headers

@szmarczak
Copy link
Member

If you're running h2c then you don't need TLS

@ronag ronag force-pushed the main branch 2 times, most recently from 6679898 to 4b5ed0b Compare July 19, 2021 07:13
@metcoder95
Copy link
Member Author

Yes sure. I'm aware of that, I was just asking if there's any open server or tool where I can do upgrades by h2c as HTTP/2 by default this runs under the secure version. If not, I'll need to create a small server by myself 😅

@metcoder95
Copy link
Member Author

How the http2client should look like? Exactly equals to Client? Or we can re-use the client implementation but just changing the way we parsed the exchange of data? I think I'm missing the big picture of that

@metcoder95
Copy link
Member Author

Just out of curiosity, after the upgrade the way how the HTTP/2 data exchange is done is quite different from an HTTP/1.1, from this point how do we proceed with the parser, is lhttp supporting it? I'm asking because the upgrade is basically done for h2c, the next step is to continue with the parsing of the different responses/requests (and of course h2).

Sorry for taking some time, I've been having rush days, I'll try to start being more concurrent next week :)

cc: @mcollina @ronag

@ronag
Copy link
Member

ronag commented Jul 26, 2021

from this point how do we proceed with the parser, is lhttp supporting it?

You should bypass all of that. llhttp does not support http2.

@metcoder95
Copy link
Member Author

Got it, just to understand once for all, reading at the issue from where this PR comes out, I do agree with all the points stated there (mostly how HTTP/2 is quite more like a transition), then my question is, what and how do we achieve HTTP/2 support?

Besides upgrade which seems straightforward, we should write a parser to accept the full HTTP/2 spec or at least the most basic part?
I'm asking because of how HTTP/2 works, the different frames for headers and body, the support for duplex, headers, etc.

Or maybe use http2 from Node internals?

The full topic is quite interesting but I do agree with the points explained in the issue 😄

@ronag
Copy link
Member

ronag commented Aug 3, 2021

You should use http2 from node internals.

@szmarczak
Copy link
Member

we should write a parser to accept the full HTTP/2 spec

I don't think we need to write another parser. It's already provided by Node.js, which is a good start. The thing is that when using h2c the server MUST reply with the response - which is kinda hard to convince the native module to accept the HTTP/2 response with the HTTP/1 request.

If I'm missing something, then it may be easier to write another parser. /cc @ronag

HTTP/2 is way more complicated than HTTP/1 so it will take definitely more time than implementing HTTP/1.

@metcoder95
Copy link
Member Author

I don't think we need to write another parser. It's already provided by Node.js, which is a good start. The thing is that when using h2c the server MUST reply with the response - which is kinda hard to convince the native module to accept the HTTP/2 response with the HTTP/1 request.

Actually, for those scenarios, I was asking for. This also includes, e.g. checks for prior requests if an HTTP 101 is response before the upgrade of protocols.

which is kinda hard to convince the native module to accept the HTTP/2 response with the HTTP/1 request.

Hmm, correct me if I'm wrong please but I think an HTTP/1.1 101 will be the response and the next exchange of data should be in the HTTP/2 protocol. Then we can "pipeline" the connection to the native module, or I'm way far?

HTTP/2 is way more complicated than HTTP/1 so it will take definitely more time than implementing HTTP/1.

Agree

@szmarczak
Copy link
Member

Then we can "pipeline" the connection to the native module, or I'm way far?

Yeah but you need to accept the response in HTTP/2 somehow without the request in HTTP/2. But the actual request is via HTTP/1. So we need to create a stream that has been already sent and is awaiting response.

@mcollina
Copy link
Member

mcollina commented Aug 4, 2021

I would recommend we focus on supporting http/2 -> http/2 before working on upgrade paths.

@jasnell have you done any research on implementing the upgrade in nodejs?

@Nytelife26
Copy link
Contributor

Nytelife26 commented Aug 4, 2021

have you done any research on implementing the upgrade in nodejs?

All information required to implement the upgrade has been summarized in the original PR, before I handed off the project.
Particularly, this comment should be of interest.

You should use http2 from node internals

Didn't we already discuss that being an idea that is somewhat antithetical to undici's purpose?
To add onto this part of the conversation, it was revealed in an earlier issue that node's native http2 would be a bottleneck given its inferior performance. If we could get a more recent version of that benchmark, that may be prevalent.

@Nytelife26
Copy link
Contributor

It seems like this PR was opened solely to get around not having write permissions for the branch of the other PR. Instead of making a duplicate, if you'd like @metcoder95 I can add you to the branch?

I'm noticing a lot of lost information and repeated discussion due to the conversations from the old PR being neglected, so that's my main concern here.

@szmarczak
Copy link
Member

szmarczak commented Aug 5, 2021

Didn't we already discuss that being an idea that is somewhat antithetical to undici's purpose?

I don't see anyone agreed to write a HTTP/2 parser from scratch.

node's native http2 would be a bottleneck given its inferior performance

I'd rather take that 20% h2 drop and prefer http/1.1 over h2 on ALPN negotiation rather to write a brand new parser for exactly this reason. I don't consider it worth the time.

@Nytelife26
Copy link
Contributor

I don't see anyone agreed to write a HTTP/2 parser from scratch.

That's what was started here. It's necessary to parse HTTP/2 transport frames in order to successfully carry out the upgrade, so we'd have to parse the frames for the upgrade anyway.

I'd rather take that 20% h2 drop and prefer http/1.1 over h2 on ALPN negotiation rather to write a brand new parser for exactly this reason. I don't consider it worth the time.

Once again, we have to do most of it anyway. It wouldn't be too much extra effort to complete it for the performance boost. After all, undici's goal is to provide a fast alternative to native.

@metcoder95
Copy link
Member Author

I would recommend we focus on supporting http/2 -> http/2 before working on upgrade paths.

By using the Node.js internal module? Then a wrapper?

All information required to implement the upgrade has been summarized in the original PR, before I handed off the project.
Particularly, this comment should be of interest.

Thanks! Actually, from the original PR, I'm basing most of the assumptions I'm doing on how the API should look, but the issue comes the way how we need to parse the frames from HTTP/2, which is where the topic gets complicated.

It seems like this PR was opened solely to get around not having write permissions for the branch of the other PR. Instead of making a duplicate, if you'd like @metcoder95 I can add you to the branch?

Yeah, sure, no issue. Is better to merge all the conversations into a single place 👍


If I can add my two cents here with my limited knowledge, I think that we should re-defined a little bit the scope of what we want to achieve. It is just to add support for the upgrade with a wrapper around the Node http2 module, I do agree that is the right step to do. The complexity is quite lower (having in mind the complexity of handling two streams at the same time somehow), and we can have the feature in place in a shorter time.
On the other hand, if we want to add full support for HTTP/2, well we should consider what's the state of the protocol towards HTTP/3 as stated in the issue linked to the PR. But if we still want to go in that direction, yeah maybe a parser from scratch will be better as otherwise we'll be just making a wrapper around it http2 (which doesn't have any issue at all but worth it to think if is really needed).
Are just my two cents, hope I'm right in something 😅

@Nytelife26
Copy link
Contributor

I invited you to the repository, so you'll be able to make changes @metcoder95.

@ronag ronag force-pushed the main branch 14 times, most recently from 5d7e3e4 to d684ce0 Compare August 18, 2021 17:27
@metcoder95 metcoder95 closed this Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants