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

Support http/2. #3730

Open
wants to merge 6 commits into
base: 5.0
Choose a base branch
from
Open

Support http/2. #3730

wants to merge 6 commits into from

Conversation

sogaani
Copy link

@sogaani sogaani commented Aug 28, 2018

This PR is rebased and added tests from #3390
We need to disable some tests on http/2, as some node module have issue with http/2. PR pending on resolution of the following issues:

@sogaani

This comment has been minimized.

@dougwilson

This comment has been minimized.

@trivikr
Copy link

trivikr commented Sep 6, 2018

Update: Node.js v10.10.0 was released an hour ago, and http2 module is no longer experimental
https://nodejs.org/en/blog/release/v10.10.0/

@dougwilson
Copy link
Contributor

So I started taking a look into this, and it seems like this is good, but only a first pass. There are still a bunch of issues. Mainly, the test suite of Express doesn't extensively test all of it's dependencies, even just the high-level ones like express.static and express.json, etc. These seem to not work correctly under the HTTP/2 implementation in this PR. I think that definitely makes the PR a bit less than ideal here.

I'm going to write up a list of the issues I'm finding. Also, it seems like there is at least one more test disabled for HTTP/2 not mentioned in the OP. We may want to add a checkmark list (I did that already) and track what is the known-outstanding issues. One of the comments in the PR is HEAD with http2 does not support response body but I'm not sure what that means -- the test is not trying to send a response body to a HEAD, as HTTP/1 doesn't support a response body in a HEAD, either.

@sogaani
Copy link
Author

sogaani commented Sep 9, 2018

One of the comments in the PR is HEAD with http2 does not support response body but I'm not sure what that means -- the test is not trying to send a response body to a HEAD, as HTTP/1 doesn't support a response body in a HEAD, either.

It's my misunderstanding. I look into it and find issue about sendFile with http2 head method.
Failing sendFile with head method is caused by difference with http_outgoing_message and http2ServerResponse. http2ServerResponse.finished with head method is true from its made, but http_outgoing_message.finished with head method is false until calling http_outgoing_message.end().

res.sendFile with head method seems to depend on the order.

  1. call onfile.
  2. call onfinish.

However, http2ServerResponse.finished is always true so that onfinish is called first.
I will fix it tomorrow.

@dougwilson

This comment has been minimized.

@sogaani

This comment has been minimized.

@dougwilson

This comment has been minimized.

@sogaani

This comment has been minimized.

@sogaani

This comment has been minimized.

@sogaani

This comment has been minimized.

@p3x-robot

This comment has been minimized.

@Abourass

This comment has been minimized.

@p3x-robot
Copy link

It looks like it only possible to work it with spdy right now.

@addaleax
Copy link

Is there anything I could do to help here?

@dougwilson
Copy link
Contributor

It turns out that even though http2 in core has a compatibility API, it is not transparent and still causes a lot of breakages. There are the obvious issues of the colon-prefixed headers that show through in req.headers and so now our internals and existing middleware need to be reworked to properly handle these (think for example anything looking at req.headers.host), but also even the events that are emitted on the objects seem to be slightly different, which is having ramifications on our deep internals.

I've pretty much come to the conclusion and started working on what I think is the only real option: create an entirely new request and response object that completely wraps the node.js core ones and evens them out between http/1 and http/2 so no module ever have to understand the difference between them. Otherwise there is going to be a growing divide in the middlewares, and just using a middleware you'll not easily be able to tell if it works with http/1, http/2, or both.

I think that the common deployment behind lbs will be http/1 for the foreseeable future, but http/2 is growing and of course is needed to provide new features, so it does news to be supposed in express. But we cannot scrasifice the usability of middleware by forcing them to manually support 1&2 and hope that just works out.

This is a large effort to create this new abstraction layer, though I have recently started work on it. It would be awesome if the core http2 compatibility api was just this layer directly, but i think with it shipping as stable that ship has sailed and express needs to stop letting the node.js core object shine through at all to ensure a stable api.

@p3x-robot
Copy link

@dougwilson so it seems, it is not just straight put in some quick http2 code, but it needs more work, but it will be actually finally be working in the foreseeable future ?

@eljefedelrodeodeljefe
Copy link

@sogaani I read the related issues (and above), can you still clarify what the blocker in the current approach is? Any chance of helping (likely no luck ): )? Cheers

@niftylettuce
Copy link

There is a core bug in HTTP/2 in Chromium/Chrome I found:

https://bugs.chromium.org/p/chromium/issues/detail?id=1045328#c50

@sogaani
Copy link
Author

sogaani commented Jun 22, 2020

@eljefedelrodeodeljefe
Referring to #3730 (comment)

Current blocker is nodejs http2 compat layer does not compatible with http module and it cause breakages when we use http2 compat layer as it is.

Current approach is create new request and response object that wrap and hide differences between http2 and http module.
Maybe, @wesleytodd work on first step of that on expressjs/discussions#82

I exposed the differences to fix them on nodejs/node#29829

@abhipanda
Copy link

@sogaani Please update the community on the progress - thank you again of spending time on this.

@tgerk
Copy link

tgerk commented Sep 24, 2021

Found this project, which may be just the right thing for non-production use: http2-express-bridge (You get what you pay for.)

@sdavids

This comment has been minimized.

@eiskalteschatten
Copy link

Any update on HTTP/2 support in Express?

@vaibhavkumar-sf
Copy link

Hey there, is express now supporting http/2 ?

@eiskalteschatten
Copy link

Really?!?! My comment was marked as spam? How about something productive like a status update instead?

@KrisLau
Copy link

KrisLau commented Oct 17, 2022

@sogaani @dougwilson Hi just wondering if this feature is still in the works / planned? Or is there is a recommended workaround?

@sogaani
Copy link
Author

sogaani commented Oct 18, 2022

I have not been working on this PR for long time. Currently I don't have much time to progress.
I would be happy if someone could take over this PR.

@sephentos
Copy link

Any update on HTTP/2 support in Express?

@LucyMaber
Copy link

what the time line on this

@mikegwhit
Copy link

mikegwhit commented Nov 8, 2023

any updates?

@sogaani mind updating the checklist above about what is left? looks like from the above list:

  • sendFile
  • cookies
  • vhost

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x enhancement needs rebase pr top priority Issues which the TC deem our current highest priorities for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet