WIP: http2: improve compatibility with http1, add tests #15633

apapirovski · 2017-09-27T01:01:31Z

This PR brings near full compatibility with express's use of http1 API. List of updates:

Add complete prop for Http2ServerRequest
Add method setter since some user-land modules use this
Add socket and connection on Http2ServerResponse
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)
Remove closure created for unsetting stream reference

Please let me know if I can provide more info for any of these changes. I know there's a lot here and some of the logic is a bit complicated in order to replicate http1 behaviour. It came a long way since my 1st pass at it.

(Re: express, after this PR only 5 tests are failing and they're a result of other modules relying on specific http1-only behaviour. This should bode well for compatibility with other popular libraries too.)

Thanks in advance for any and all reviews!

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

jasnell · 2017-09-28T12:24:56Z

lib/internal/http2/compat.js

@@ -234,6 +235,10 @@ class Http2ServerRequest extends Readable {
    return this[kHeaders][HTTP2_HEADER_METHOD];
  }

+  set method(method) {
+    this[kHeaders][HTTP2_HEADER_METHOD] = method;


should this include a basic type check to ensure that method is a non-empty string?

Good point. http1 obviously didn't have it since it's not a getter/setter but I don't see why we couldn't do that for http2.

jasnell · 2017-09-28T12:26:47Z

lib/internal/http2/compat.js

-  get closed() {
-    return this[kState].closed;
+
+  get socket() {


I'm wondering if we shouldn't more proactively warn people away from accessing this... e.g. by using an explicit runtime deprecation warning right from the start.

Similar to statusMessageWarn? We could definitely do that.

yep, exactly like that.

As long as there is a way to retrieve the remote IP address for the socket (and local port, etc.); ideally symmetrically with http/1.

I think we will keep the socket available but just add a one-time warning re: its usage.

Good point and a good reminder! I had that marked down quite some time ago as a todo for an API that would make fetching those details easier but hadn't gotten around to it. We can make that data available via the Http2Session object so that the socket property does not have to be accessed.

I think we will keep the socket available but just add a one-time warning re: its usage.

So getting the remote IP will print a warning?

just getting it via the request and response objects should print a warning. We should add apis that allow this information to be retrieved independently of that so that there's a path that (a) does not print the warning and (b) does not mean users are accessing the socket directly.

Sounds good. Would that land prior to landing a warning on the socket getter, then?

We could probably do those at the same time actually.

Extensive rework of Http2ServerResponse behaviours based on the expressjs test suite in order to get as close as possible to mimicking http1. Adds tests for all added and changed behaviour.

apapirovski · 2017-09-28T13:08:38Z

Made the requested changes. Will work on adding the ability to access the address info next (within this PR) unless @jasnell would prefer to do it or prefers it separated out into a new PR.

On that note, we can't add 'session' getter to request or response (conflicts with existing user-land modules), do we just make users get it via request.stream.session or different alias or do we expose the address stuff on the stream too?

jasnell · 2017-09-28T13:14:54Z

Go for it! I'm going to be tied up in meetings and flights for most of the day so definitely have at it and don't wait on me! :-)

apapirovski · 2017-09-29T12:52:24Z

This is undergoing somewhat major changes atm so please no reviews for now as it'll need to be reviewed after the next commit. I'm working on a solution for the socket problem and we should be able to get http2 to work with on-finished as a result (which is the biggest blocker for full express compatibility).

I will in all likelihood open a new PR once that work is done (and close this one, putting it in Ref for the commit) since a lot of the changes made here will look different + there'll be new ones.

apapirovski · 2017-09-29T22:48:38Z

I have basically 100% compatibility with express, on-finished and finalhandler now (all tests are passing). Working on test cases covering all the edge cases, might take a day or two before I have a PR — depending on my schedule.

(I'm using a proxy object for the socket and applying things like 'on', 'emit', 'readable', etc. to the Http2Stream instead while anything unaccounted for hits the socket. As a result of some other changes, performance for compatibility mode actually improved despite the usage of a Proxy.)

Closing this now. Will open a new PR when done.

mcollina · 2017-09-30T08:16:12Z

I think you inadvertently pushed the closed button, I'm reopening.

mcollina · 2017-09-30T08:37:21Z

Oh my bad, I didn't read the last sentence

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

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>

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>

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Sep 27, 2017

jasnell requested a review from mcollina September 28, 2017 12:23

jasnell reviewed Sep 28, 2017

View reviewed changes

apapirovski added 2 commits September 28, 2017 08:34

http2: improve compatibility with http1, add tests

2e32a51

Extensive rework of Http2ServerResponse behaviours based on the expressjs test suite in order to get as close as possible to mimicking http1. Adds tests for all added and changed behaviour.

squash: add method setter validation & warn on accessing socket

680e507

apapirovski force-pushed the patch-http2-compat-head-and-more branch from 258f895 to 680e507 Compare September 28, 2017 13:06

apapirovski changed the title ~~http2: improve compatibility with http1, add tests~~ WIP: http2: improve compatibility with http1, add tests Sep 29, 2017

apapirovski closed this Sep 29, 2017

mcollina reopened this Sep 30, 2017

mcollina closed this Sep 30, 2017

apapirovski deleted the patch-http2-compat-head-and-more branch October 14, 2017 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: http2: improve compatibility with http1, add tests #15633

WIP: http2: improve compatibility with http1, add tests #15633

apapirovski commented Sep 27, 2017 •

edited

Loading

jasnell Sep 28, 2017

apapirovski Sep 28, 2017 •

edited

Loading

jasnell Sep 28, 2017

apapirovski Sep 28, 2017

jasnell Sep 28, 2017

dougwilson Sep 28, 2017

apapirovski Sep 28, 2017

jasnell Sep 28, 2017

dougwilson Sep 28, 2017

jasnell Sep 28, 2017

dougwilson Sep 28, 2017

jasnell Sep 28, 2017

apapirovski commented Sep 28, 2017 •

edited

Loading

jasnell commented Sep 28, 2017

apapirovski commented Sep 29, 2017 •

edited

Loading

apapirovski commented Sep 29, 2017 •

edited

Loading

mcollina commented Sep 30, 2017

mcollina commented Sep 30, 2017

WIP: http2: improve compatibility with http1, add tests #15633

WIP: http2: improve compatibility with http1, add tests #15633

Conversation

apapirovski commented Sep 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

apapirovski Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Sep 28, 2017 • edited Loading

jasnell commented Sep 28, 2017

apapirovski commented Sep 29, 2017 • edited Loading

apapirovski commented Sep 29, 2017 • edited Loading

mcollina commented Sep 30, 2017

mcollina commented Sep 30, 2017

apapirovski commented Sep 27, 2017 •

edited

Loading

apapirovski Sep 28, 2017 •

edited

Loading

apapirovski commented Sep 28, 2017 •

edited

Loading

apapirovski commented Sep 29, 2017 •

edited

Loading

apapirovski commented Sep 29, 2017 •

edited

Loading