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

http2: near full http1 compatibility, add tests #15702

Closed

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Sep 30, 2017

As a preface, this is a lot of changes but I had no way to separate them out into small chunks due to the order in which they happened. I started with working on support for HEAD requests and other minor http2 tweaks but as I got the express & on-finished test suites to work, I had to change a ton of that code and as a result it's hard for me to separate it out since some of these changes have broader effect on the compatibility layer.

With this PR we pass 100% of tests in express & on-finished that are applicable to http2. (There are tests in on-finished that don't apply re: handling upgrade and there are 4 tests in express that use a 3rd party module that we can't do anything about because it uses http1 prototype directly.)

Full or near full list of changes follows:

  • Add complete prop for Http2ServerRequest
  • Add method setter since some user-land modules use this
  • Add socket and connection on Http2ServerResponse
  • Add a proxy socket that is in reality referring to the Http2Stream for all the most commonly used aspects of the socket in http1
  • Major rework of writeHead, write, end, addHeader and removeHeader. A lot of user-land modules, including express, rely on the fact that in http1 all write methods call writeHead if headers haven't been written yet.
  • Add support for http1 style handling of HEAD requests (expects end call and triggers callback, even if writeHead already happened)
  • Dump data in Readable when kFinish runs if the user hasn't accessed it yet, this is what h1 does
  • No longer unset kStream and instead unset kResponse & kRequest
  • no longer emit an error event for the CANCEL rst code (8) — this is done mostly for h1 compatibility but also because a lot of clients seem to use this to signify client-requested abort (similar to how you could just trigger shutdown with code 0). The code is still emitted on streamClosed if anyone wants to know. Since the error emits on stream aren't actionable in http2, it seems like this has very minor implications and it allows us to support the user code already out there much easier. I could potentially just do some manipulation in my proxy socket instead but it really hampers its performance and as a result the performance of express & on-finished.
  • Separate commit that adds address related info on the session instead of needing to access the socket.

Performance has actually improved by about 5% with all of the changes in here, even when accessing the socket — despite the use of a Proxy.

Please let me know if I can provide more info for any of these changes. It came a long way since I started this work.

cc @jasnell, @mcollina & @dougwilson

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2, test

@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Sep 30, 2017
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

This is a lot to digest, I'll do my best to review asap, but it might happen in a while. Can you also update the docs accordingly? I think we should document what you are doing with the Proxy.

👍 on the Express compatibility.

@apapirovski
Copy link
Member Author

apapirovski commented Oct 1, 2017

Thanks @mcollina! Pushed a tiny change that got rid of the unnecessary createProxySocket function (used to do more in it, now I can just call new Proxy directly) and also added setTimeout handler for the proxy (which sets a session timeout) since that's the one other common use case. Update: Also correctly binding socket functions now.

Will work on docs and add a commit either later today or sometime the coming week.


get code() {
return this[kState].closedCode;
get complete() {
Copy link
Member

Choose a reason for hiding this comment

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

What's this? I can't see this is a part of the official h1 api?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not documented but libraries use it.

@@ -387,9 +477,16 @@ class Http2ServerResponse extends Stream {
if (typeof name !== 'string')
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');

if (this[kStream].headersSent)
throw new errors.Error('ERR_HTTP2_HEADERS_SENT');

Copy link
Member

Choose a reason for hiding this comment

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

We check that name is valid but not value?

Copy link
Member Author

@apapirovski apapirovski Oct 1, 2017

Choose a reason for hiding this comment

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

assertValidHeader(name, value); takes care of value. We're going to properly validate name & value further downstream at some point (outside of compat), not yet implemented.

@@ -71,19 +84,24 @@ function statusMessageWarn() {
}

function onStreamData(chunk) {
if (!this[kRequest].push(chunk))
const request = this[kRequest];
if (request !== undefined && !request.push(chunk))
Copy link
Member

Choose a reason for hiding this comment

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

Should request === undefined even be possible here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically? Yes. Likely? No.

@@ -823,6 +822,44 @@ class Http2Session extends EventEmitter {
return settings;
}

// Proxies for socket address-related getters so users don't
// ever need to reach through to the socket
get remoteAddress() {
Copy link
Member

Choose a reason for hiding this comment

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

These need to be added to the docs in /doc/api/http2.md. Should be done in this PR but can be done in a separate one if necessary

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Code changes LGTM with green CI.
Would prefer docs to be updated in this PR tho.

Really great to see this!

@apapirovski
Copy link
Member Author

apapirovski commented Oct 2, 2017

@jasnell I've been thinking over the socket warning a bit more and I'm wondering if we really should have it or not... There are some legitimate reasons to access the socket, such as with the address-related stuff but also for TLS, for stuff like getPeerCertificate(), getSession(), etc.

Realistically, it's not like we want to maintain the whole range of socket-related getters, setters and methods on the session. And it seems somewhat arbitrary as to what we actually expose on the session and what we don't.

I'm personally almost leaning towards removing the 2nd commit (http2: add socket address getters on session) and adjusting the warning to only appear if users try to use stuff like resume, pause, read, write or end on the socket proxy. Thoughts?

@jasnell
Copy link
Member

jasnell commented Oct 2, 2017

Yeah, I know getting to this information is important, it's just a really bad idea to expose the socket easily with http2 connections. We've seen things like that come back and bite us in the past. Let me stew on this a bit more to see if I can think of a reasonable alternative... if there's not, then I'm good with dropping the warning for now.

@apapirovski
Copy link
Member Author

apapirovski commented Oct 2, 2017

Sounds good. I think my preferred option is to have the warning just for certain methods as mentioned above (the ones that are actually destructive) but I realize that would only apply to compatibility mode and anyone using the proper http2 API wouldn't hear a peep.

@mcollina
Copy link
Member

mcollina commented Oct 3, 2017

I'm 👍 on having the warning on some of the stream-related APIs. In fact, I would not make it a warning, but I'd rather throw. If you are calling one of those methods you definitely have a bug.

Extensive re-work of http1 compatibility layer based on tests in
express, on-finished and finalhandler. Fix handling of HEAD
method to match http1. Adjust write, end, etc. to call writeHead
as in http1 and as expected by user-land modules. Add socket
proxy that instead uses the Http2Stream for the vast majority of
socket interactions. Add and change tests to closer represent
http1 behaviour.

Refs: nodejs#15633
Refs: https://github.com/expressjs/express/tree/master/test
Refs: https://github.com/jshttp/on-finished/blob/master/test/test.js
Refs: https://github.com/pillarjs/finalhandler/blob/master/test/test.js
@apapirovski
Copy link
Member Author

Ok, I've got an updated version ready to go just in case. It throws ERR_HTTP2_NO_SOCKET_MANIPULATION with the following message

HTTP/2 sockets should not be directly read from, written to, paused and/or resumed.

if the user tries to read, write, pause or resume the request.socket or response.socket. I added end to the list of things that are called on stream since hapi seems to use it.

It also removes the socket warning and the socket address related getters. Will commit or adjust further once there's consensus.

Working on the docs at the moment so should have those ready sometime tomorrow.

@apapirovski
Copy link
Member Author

Ok, I've force pushed a commit with updates as per above and a new commit with the doc changes (plus some formatting fixes as we had the wrong heading levels for a lot of the compatibility stuff).

@jasnell
Copy link
Member

jasnell commented Oct 3, 2017

Still LGTM with green CI :-)

@benjamingr
Copy link
Member

@mcollina
Copy link
Member

mcollina commented Oct 6, 2017

Landed as 2da7d9b.

@mcollina mcollina closed this Oct 6, 2017
mcollina pushed a commit that referenced this pull request Oct 6, 2017
Extensive re-work of http1 compatibility layer based on tests in
express, on-finished and finalhandler. Fix handling of HEAD
method to match http1. Adjust write, end, etc. to call writeHead
as in http1 and as expected by user-land modules. Add socket
proxy that instead uses the Http2Stream for the vast majority of
socket interactions. Add and change tests to closer represent
http1 behaviour.

Refs: #15633
Refs: https://github.com/expressjs/express/tree/master/test
Refs: https://github.com/jshttp/on-finished/blob/master/test/test.js
Refs: https://github.com/pillarjs/finalhandler/blob/master/test/test.js
PR-URL: #15702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 7, 2017
Extensive re-work of http1 compatibility layer based on tests in
express, on-finished and finalhandler. Fix handling of HEAD
method to match http1. Adjust write, end, etc. to call writeHead
as in http1 and as expected by user-land modules. Add socket
proxy that instead uses the Http2Stream for the vast majority of
socket interactions. Add and change tests to closer represent
http1 behaviour.

Refs: #15633
Refs: https://github.com/expressjs/express/tree/master/test
Refs: https://github.com/jshttp/on-finished/blob/master/test/test.js
Refs: https://github.com/pillarjs/finalhandler/blob/master/test/test.js
PR-URL: #15702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 7, 2017
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
Extensive re-work of http1 compatibility layer based on tests in
express, on-finished and finalhandler. Fix handling of HEAD
method to match http1. Adjust write, end, etc. to call writeHead
as in http1 and as expected by user-land modules. Add socket
proxy that instead uses the Http2Stream for the vast majority of
socket interactions. Add and change tests to closer represent
http1 behaviour.

Refs: #15633
Refs: https://github.com/expressjs/express/tree/master/test
Refs: https://github.com/jshttp/on-finished/blob/master/test/test.js
Refs: https://github.com/pillarjs/finalhandler/blob/master/test/test.js
PR-URL: #15702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
Extensive re-work of http1 compatibility layer based on tests in
express, on-finished and finalhandler. Fix handling of HEAD
method to match http1. Adjust write, end, etc. to call writeHead
as in http1 and as expected by user-land modules. Add socket
proxy that instead uses the Http2Stream for the vast majority of
socket interactions. Add and change tests to closer represent
http1 behaviour.

Refs: nodejs/node#15633
Refs: https://github.com/expressjs/express/tree/master/test
Refs: https://github.com/jshttp/on-finished/blob/master/test/test.js
Refs: https://github.com/pillarjs/finalhandler/blob/master/test/test.js
PR-URL: nodejs/node#15702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@apapirovski apapirovski deleted the patch-http2-compat-http1 branch October 14, 2017 16:04
@mcollina mcollina mentioned this pull request May 16, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants