Initial support proposal for http2 #3390

phouri · 2017-08-08T11:11:52Z

Hi,

PiniH here - used wrong account before (work...).

Regarding this issue: #3388

This is an initial proposal for making http2 work, this requires node to expose the Http2ServerRequest/Response and creates both request/response on the app and attaches the correct one depending on the request.

This is just an initial proposal (and a working example if Http2ServerRequest/Response) is exposed.

In order for express (and possibly other libraries) to get and use the Http2ServerRequest/Response - expose them in the http2 exports. Same as is done in http module. This is only a suggestion - not sure if this is the way to go, but with this it allows for express to run with http2 server immediately. ref: expressjs/express#3390 fixes: nodejs#14672 Fix Lint

phouri · 2017-08-11T08:28:33Z

@dougwilson once I clear the PR on nodeJS I will continue working on this if you'd like.

My gut feeling is that there will be a need to split it into 2 separate files - httpRequest and http2Request (I just did a quick work here to make it work, I don't think it's the right approach) - and take out all the special decorators into another file and use them accordingly.

WDYT?

In order for express (and possibly other libraries) to get and use the Http2ServerRequest/Response - expose them in the http2 exports. Same as is done in http module. PR-URL: #14690 Ref: expressjs/express#3390 Fixes: #14672 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

In order for express (and possibly other libraries) to get and use the Http2ServerRequest/Response - expose them in the http2 exports. Same as is done in http module. PR-URL: nodejs/node#14690 Ref: expressjs/express#3390 Fixes: nodejs/node#14672 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PiniH · 2017-08-31T06:03:17Z

@dougwilson Hey, any thoughts regarding this? now that the PR in node is in we can move forward on this end :)

dougwilson · 2017-08-31T12:37:02Z

Seems to be in the right direction. Probably need to have CI run test suite twice: once for HTTP/1 and once for HTTP/2.

Also the test suite here does leave a lot of the fine details up to the dependencies like send, on-headers, on-finished, finalhandler, etc. and they all need their test suites run against thr HTTP/2 server to make sure they still work right.

dougwilson · 2017-08-31T12:45:04Z

P.S. I added the "5.x" label for now since this PR is changing exported API in a non-backwards-compatible way. That's not a bad thing, becasuse a 5.0 for this is fine, just wanted to note the reason.

dougwilson · 2017-08-31T12:58:38Z

lib/request.js

+   * @return {String}
+   * @public
+   */
+  if (req instanceof http.IncomingMessage) {


So this is not going to make a compatible API. In Express, req.path is the pathname of thr URL (which, for instance, does not include the query stribg). Th HTTP2 prototype just aliases req.url to req.path, meaning they are the same and the req.path for HTTP2 would include the query string.

If Node.js code is going to insist on keep this change, then this could never land prior to 5.0 and req.path needs to go through a drprecation cycle in 4.x as well. Then this implementation can ignore req.path.

I see,

What if we override this here and return the pathname like http1 does?
This will however create inconsistencies between http2 and http1, not sure what's better.

If it's possible to do, absolutely! If you can't,
please let James in thr Node.js issue know as well. Consistency in the Express.js API is what you are promised when you use Express.js, so I'm not really understanding what inconsistency you mean would come from this. Can you explain more?

Actually the inconsistency I thought about would be between the IncomingMessage and Http2ServerMessage but you are right - this is not an issue if you use express and you expect express stuff :)

I will add an override that will return the pathname like normal req.path does in express in the weekend.

I will also work on reverting the api change, get all the common decorators into a util helper and create new requestHttp2.js files instead of same file returning both requests/responses.

Ah, gotcha. Yea, even the http1 class has slightly changed across Node.js versions. To that end we don't document the Node.js given APIs and just tell users to refer to the docs for their Node.js version.

Regarding the refactor: cool. The changes made to the test/exports file is the indication that the PR was not backwards-compatible, if you're wondering how I could tell :)

phouri · 2017-08-31T13:01:17Z

I tried looking up how to make the test utils use http2 client instead of http1, seems like it's not supported at the moment: ladjs/supertest#429

As to breaking the API - I thought that maybe I can rewrite this to make separate request and requestHttp2 files - and put the common decorators in a common code, that way it won't break existing API.

As to increasing the testing suites, I think I will need a little help with that :)

phouri · 2017-08-31T20:09:51Z

Yep, reverted all the changes I did to the test, expect a pr tomorrow, still not sure how to accomplish all the tests.

phouri · 2017-09-01T09:32:47Z

Finished the refactor for this.

Moved all the code that was inside request.js / response.js to the requestDecorator/responseDecorator.

request.js and response.js now again returns the httpRequest without breaking the api.

Adjusted the req.path on http2 to return only pathname - had to create a new mini object for the parseurl library (it only ever used the url part of the request, and cached the results).

@dougwilson LMK what you think

wesleytodd · 2018-01-23T19:09:27Z

Hey, I have not been involved in this discussion, just reading along mostly, but I was wondering if a better overall approach would be removing the prototype extension and replace it with our own custom req/res objects?

There has been discussion about doing that anyway, and it would greatly simplify this pull request I think. What do people think about doing that before we finalize this?

phouri · 2018-01-24T09:22:35Z

This PR was supposed to be a relatively small change that will also support http2.
Refactoring req/res sounds like a big change, and will make this redundant 👍

wesleytodd · 2018-01-24T16:45:06Z

Not redundant, just that the work you did here would also need to be applied there. I am unsure if this could have been done in a way which didn't break backward compatibility, so it was probably going to only land in 5.x anyway. With that in mind, it seems like resolving these then applying this on top is the best way forward:

#2432
#3214

atdiff · 2018-04-04T15:35:39Z

@phouri any updates on when this PR will be finished and the http2 functionality available?

therealcritiq · 2018-05-16T01:13:55Z

bump @atdiff's question.

if work on this is paused, what are some workarounds or alternative solutions that people are using?

dougwilson · 2018-05-16T01:18:17Z

I'm happy to pick up the work that was left off. I asked if I could get a copy in #3390 (comment) but never received anything so far. Anyone is welcome to pick this up as well, and I don't have a lot of time to put into this which is why I was really hoping to get that code.

sogaani · 2018-07-29T02:06:43Z

Hello @dougwilson
I'm working on this PR.
My forked branch have been working tests with http2. Would you review my codes and help me to apply my commit on this PR?

Run tests with following commands.

npm install
npm run test-http2

I changed following packages to support http2.
We need to apply PR those packages first.

Other modified packages.
- superagent
- supertest
- cookies
- cookie-session
- vhost

benjamingr · 2018-07-29T07:37:40Z

cc @phouri

phouri · 2018-07-30T12:20:03Z

Feel free to take over this PR and continue what ever is needed, I unfortunately do not have time for this :(

phouri · 2018-07-30T12:20:13Z

Can close this if needed

sogaani · 2018-08-28T12:41:02Z

I create PR #3730 to rebase and add tests.

dougwilson · 2018-11-06T18:18:48Z

Since the pull request has merge conflicts and the author asked for it to be closed and a new pull request was created to take it's place, I am closing this now that it is effectively a duplicate and the author won't be moving it forward anyhow.

phouri force-pushed the initial-support-http2 branch 4 times, most recently from 1798c75 to 5d22430 Compare August 8, 2017 13:18

phouri mentioned this pull request Aug 8, 2017

http2: Expose Http2ServerRequest/Response nodejs/node#14690

Closed

2 tasks

dougwilson added the pr label Aug 8, 2017

phouri force-pushed the initial-support-http2 branch 2 times, most recently from 8644c2f to fc61715 Compare August 9, 2017 13:57

Initial support proposal for http2

58b88f1

phouri force-pushed the initial-support-http2 branch from fc61715 to 58b88f1 Compare August 9, 2017 13:59

tamaina mentioned this pull request Aug 27, 2017

HTTP/2 #700 misskey-dev/misskey#733

Closed

dougwilson added 5.x enhancement needs tests labels Aug 31, 2017

dougwilson reviewed Aug 31, 2017

View reviewed changes

phouri added 3 commits September 1, 2017 12:11

Seperate to files and revert breaking api

43f7c01

Require parseurl and don't emit warning on setter

f98568f

Lint

17efcbf

Http2 Example

b9276c7

dougwilson mentioned this pull request Jan 24, 2018

Node http2 - cannot read this.socket.readable #3388

Open

BernzSed mentioned this pull request Feb 9, 2018

How to use it with RouterContext ? BernzSed/axios-push#1

Closed

wesleytodd mentioned this pull request Mar 19, 2018

Discussion: Exit criteria for bringing http2 out of experimental nodejs/node#19453

Closed

5 tasks

dougwilson mentioned this pull request May 5, 2018

HTTP/2 in express for node v10 #3644

Closed

cainrus mentioned this pull request Jul 11, 2018

Support HTTP/2 webpack-contrib/webpack-hot-middleware#322

Merged

6 tasks

This was referenced Jul 30, 2018

Support Http2 pillarjs/cookies#99

Open

Support Http2 expressjs/vhost#29

Open

Support http2. ladjs/superagent#1399

Merged

addaleax mentioned this pull request Aug 1, 2018

fix(Server): don't use spdy on node >= v10.0.0 webpack/webpack-dev-server#1451

Merged

5 tasks

This was referenced Aug 2, 2018

Support http2. ladjs/supertest#494

Closed

Support http2 jshttp/type-is#36

Open

sogaani mentioned this pull request Aug 28, 2018

Support http/2. #3730

Open

5 tasks

sogaani mentioned this pull request Aug 29, 2018

Support http2 WebThingsIO/gateway#1145

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support proposal for http2 #3390

Initial support proposal for http2 #3390

phouri commented Aug 8, 2017

phouri commented Aug 11, 2017

PiniH commented Aug 31, 2017

dougwilson commented Aug 31, 2017

dougwilson commented Aug 31, 2017

dougwilson Aug 31, 2017

phouri Aug 31, 2017

dougwilson Aug 31, 2017

phouri Aug 31, 2017

phouri Aug 31, 2017

dougwilson Aug 31, 2017

dougwilson Aug 31, 2017

phouri commented Aug 31, 2017

phouri commented Aug 31, 2017 via email •

edited by dougwilson

phouri commented Sep 1, 2017

wesleytodd commented Jan 23, 2018

phouri commented Jan 24, 2018

wesleytodd commented Jan 24, 2018

atdiff commented Apr 4, 2018 •

edited

therealcritiq commented May 16, 2018

dougwilson commented May 16, 2018

sogaani commented Jul 29, 2018

benjamingr commented Jul 29, 2018

phouri commented Jul 30, 2018

phouri commented Jul 30, 2018

sogaani commented Aug 28, 2018

This comment has been minimized.

This comment has been minimized.

dougwilson commented Nov 6, 2018

Initial support proposal for http2 #3390

Initial support proposal for http2 #3390

Conversation

phouri commented Aug 8, 2017

phouri commented Aug 11, 2017

PiniH commented Aug 31, 2017

dougwilson commented Aug 31, 2017

dougwilson commented Aug 31, 2017

dougwilson Aug 31, 2017

Choose a reason for hiding this comment

phouri Aug 31, 2017

Choose a reason for hiding this comment

dougwilson Aug 31, 2017

Choose a reason for hiding this comment

phouri Aug 31, 2017

Choose a reason for hiding this comment

phouri Aug 31, 2017

Choose a reason for hiding this comment

dougwilson Aug 31, 2017

Choose a reason for hiding this comment

dougwilson Aug 31, 2017

Choose a reason for hiding this comment

phouri commented Aug 31, 2017

phouri commented Aug 31, 2017 via email • edited by dougwilson

phouri commented Sep 1, 2017

wesleytodd commented Jan 23, 2018

phouri commented Jan 24, 2018

wesleytodd commented Jan 24, 2018

atdiff commented Apr 4, 2018 • edited

therealcritiq commented May 16, 2018

dougwilson commented May 16, 2018

sogaani commented Jul 29, 2018

benjamingr commented Jul 29, 2018

phouri commented Jul 30, 2018

phouri commented Jul 30, 2018

sogaani commented Aug 28, 2018

This comment has been minimized.

This comment has been minimized.

dougwilson commented Nov 6, 2018

phouri commented Aug 31, 2017 via email •

edited by dougwilson

atdiff commented Apr 4, 2018 •

edited