Skip to content

Support http/2.#3730

Open
sogaani wants to merge 6 commits intoexpressjs:5.0from
sogaani:http2-support
Open

Support http/2.#3730
sogaani wants to merge 6 commits intoexpressjs:5.0from
sogaani:http2-support

Conversation

@sogaani
Copy link
Copy Markdown

@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
Copy Markdown

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

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

@addaleax
Copy link
Copy Markdown

Is there anything I could do to help here?

@dougwilson
Copy link
Copy Markdown
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
Copy Markdown

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

@tgerk
Copy link
Copy Markdown

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

Any update on HTTP/2 support in Express?

@vaibhavkumar-sf
Copy link
Copy Markdown

Hey there, is express now supporting http/2 ?

@eiskalteschatten
Copy link
Copy Markdown

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

@KrisLau
Copy link
Copy Markdown

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

Any update on HTTP/2 support in Express?

@LucyMaber
Copy link
Copy Markdown

what the time line on this

@mikegwhit
Copy link
Copy Markdown

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

@nidhishgajjar
Copy link
Copy Markdown

Orb Code Review (powered by GLM-4.7 on Orb Cloud)## SummaryThis PR adds HTTP/2 support to Express.js by introducing a dual prototype system where HTTP/2 requests/responses use separate prototypes that share the same functionality via decorator pattern. The implementation adds ~98 files including new HTTP/2-specific modules, test infrastructure, and examples.## ArchitectureThe PR introduces a decorator pattern to separate request/response methods from their prototypes, allowing the same methods to be applied to both HTTP/1 (IncomingMessage/ServerResponse) and HTTP/2 (Http2ServerRequest/Http2ServerResponse) objects. The application handler dynamically switches prototypes based on request type using instanceof checks. Testing infrastructure wraps supertest to conditionally use HTTP/2 when HTTP2_TEST environment variable is set.## Issuesexamples/http2/keys/test_cert.pem, examples/http2/keys/test_key.pem — severity [warning] — security concern with hardcoded test certificatesThe PR adds SSL/TLS certificates directly to the repository. While these appear to be test certificates, hardcoding cryptographic keys in version control is generally discouraged. Consider using certificate generation in test setup or documenting that these are for demonstration purposes only.lib/application.js:245-250 — severity [warning] — runtime prototype switching may impact performanceThe PR switches request/response prototypes on every request based on instanceof checks. This runtime type checking and prototype manipulation adds overhead to the request handling path, which is a hot path in Express.lib/application.js:261-266 — severity [suggestion] — prototype restoration in mounted apps could be simplifiedThe prototype restoration logic in mounted apps duplicates the type checking logic. Consider extracting this to a helper function to reduce code duplication and potential for inconsistency.lib/http2Request.js:352-367 — severity [warning] — HTTP/2 host handling assumes connection property existsThe HTTP/2 host property getter uses this.connection.remoteAddress without checking if the connection property exists or has the expected structure. HTTP/2 streams may have different connection properties than HTTP/1.test/support/supertest.js — severity [suggestion] — HTTP/2 test infrastructure lacks documentationThe custom supertest wrapper for HTTP/2 testing is not well documented. Consider adding comments explaining when and how to use HTTP/2 tests, and what the HTTP2_TEST environment variable does.package.json:78 — severity [suggestion] — supertest version bump should be explainedThe PR bumps supertest from 1.2.0 to 2.0. This is a major version bump that may introduce breaking changes. The commit message or PR description should explain why this upgrade is necessary for HTTP/2 support.## Cross-file ImpactPublic API Changes:- express.isHttp2Supported - new boolean flag exported- express.http2Request - new HTTP/2 request prototype exported - express.http2Response - new HTTP/2 response prototype exportedBreaking Changes: None explicitly, but the supertest upgrade (1.2.0 → 2.0) may affect users who pin exact versions.Test Impact: All existing tests are modified to use the local supertest wrapper (./support/supertest) instead of the direct supertest package. This ensures tests can run against both HTTP/1 and HTTP/2, but increases the surface area for potential regressions.Performance Impact: Runtime prototype switching on every request adds overhead. The dual prototype system increases memory footprint as both HTTP/1 and HTTP/2 prototypes are maintained even if only one protocol is used.## Assessment⚠️ Request changesThis is a substantial feature addition that introduces significant architectural complexity. While the HTTP/2 support is valuable, the implementation has several concerns:1. Security: Hardcoded test certificates in the repository2. Performance: Runtime prototype switching on every request3. Complexity: Dual prototype system increases maintenance burden4. Testing scope: 98 files changed is very large for a single PR5. Age: PR from 2018 still open in 2024 suggests ongoing merge issuesConsider splitting this into smaller, focused PRs:- PR 1: Refactor request/response to decorator pattern (backward compatible)- PR 2: Add HTTP/2 support with minimal core changes- PR 3: Update test infrastructure separatelyThe HTTP/2 feature is important for Express, but the current implementation would benefit from simplification and better separation of concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.