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

Initial support proposal for http2 #3390

Closed
wants to merge 12 commits into from

Conversation

phouri
Copy link

@phouri phouri commented Aug 8, 2017

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.

@phouri phouri force-pushed the initial-support-http2 branch 4 times, most recently from 1798c75 to 5d22430 Compare August 8, 2017 13:18
@dougwilson dougwilson added the pr label Aug 8, 2017
@phouri phouri force-pushed the initial-support-http2 branch 2 times, most recently from 8644c2f to fc61715 Compare August 9, 2017 13:57
phouri added a commit to phouri/node that referenced this pull request Aug 10, 2017
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
Copy link
Author

phouri commented Aug 11, 2017

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

benjamingr pushed a commit to nodejs/node that referenced this pull request Aug 16, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@PiniH
Copy link

PiniH commented Aug 31, 2017

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

@dougwilson
Copy link
Contributor

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
Copy link
Contributor

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.

lib/request.js Outdated
* @return {String}
* @public
*/
if (req instanceof http.IncomingMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

phouri commented Aug 31, 2017

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
Copy link
Author

phouri commented Aug 31, 2017 via email

@phouri
Copy link
Author

phouri commented Sep 1, 2017

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
Copy link
Member

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
Copy link
Author

phouri commented Jan 24, 2018

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
Copy link
Member

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

atdiff commented Apr 4, 2018

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

@therealcritiq
Copy link

bump @atdiff's question.

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

@dougwilson
Copy link
Contributor

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

sogaani commented Jul 29, 2018

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.

@benjamingr
Copy link

cc @phouri

@phouri
Copy link
Author

phouri commented Jul 30, 2018

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

@phouri
Copy link
Author

phouri commented Jul 30, 2018

Can close this if needed

@sogaani
Copy link

sogaani commented Aug 28, 2018

I create PR #3730 to rebase and add tests.

@p3x-robot

This comment has been minimized.

@Abourass

This comment has been minimized.

@dougwilson
Copy link
Contributor

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.

@dougwilson dougwilson closed this Nov 6, 2018
@expressjs expressjs locked and limited conversation to collaborators Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.