http2: minor cleanup of compat, tests #15254

apapirovski · 2017-09-08T01:15:20Z

This is a bit of a hodgepodge of stuff that's too small for its own PRs. Here's the full list:

Remove path getter and setter, related to Initial support proposal for http2 expressjs/express#3390.
Add validation for header names and values, as per the spec. @mcollina I would like your input on this as doing this in compat means that users can still set invalid headers through the API in core. It doesn't feel like we want this in two places but if we only validate in mapToHeaders then methods like setHeader won't throw when they probably should. I'm also not sure if I should be importing code from http or making a copy in http2 utils — not sure how node would usually handle this.
A few of the validations within if statements were moved up, such as checking for stream.destroyed in kBeginSend to avoid executing unnecessary code.
Remove unused properties in kState.
Remove all the extra code for creating kHeaders & kTrailers and instead just create them in the constructor. I ran a benchmark and this had no performance impact but it keeps the code a lot cleaner since we don't have to keep constantly checking for it.
Cleanup unnecessary variable assignments throughout to match the majority of the code in h2 and node in general.

Let me know if there's anything I can adjust! Thanks.

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

mcollina

Good work, thanks so much for helping out. I've left some comments around.

Regarding headers, I think we might want to validate them in core.js, but I'll leave the final decision to @jasnell.

While you are cleaning things up, I think the doc for Http2ServerRequest is not completed (my bad), would you mind to fill it up? https://nodejs.org/api/http2.html#http2_class_http2_http2serverrequest

mcollina · 2017-09-08T04:30:22Z

lib/internal/http2/compat.js

+    case HTTP2_HEADER_METHOD:
+    case HTTP2_HEADER_PATH:
+    case HTTP2_HEADER_AUTHORITY:
+    case HTTP2_HEADER_SCHEME:


these read worse than the string IMHO. I'm ok in doing thia, just add a small comment nearby with their meaning.

Yeah, I just wanted to have this similar to core where everything is a constant. I'll add the comments for sure.

mcollina · 2017-09-08T05:01:18Z

lib/internal/http2/compat.js

@@ -155,23 +167,17 @@ class Http2ServerRequest extends Readable {
  }

  get closed() {
-    const state = this[kState];
-    return Boolean(state.closed);
+    return this[kState].closed;


I think kState can be nulled, shouldn't we protect this?

I don't think there's ever a situation where that can happen (I reviewed all of the code in compat) and since it's accessed via a Symbol, users would have to really go out of their way to null it. (And we only ever set this to true within kFinish, no other manipulation.)

Ok, good catch!

mcollina · 2017-09-08T05:01:40Z

lib/internal/http2/compat.js

@@ -201,7 +207,7 @@ class Http2ServerRequest extends Readable {
  }

  get socket() {
-    return this.stream.session.socket;
+    return this[kStream].session.socket;


this[kStream] can be nulled, shouldn't we protect this?

Yeah, I agree. It wasn't protected before so I wasn't sure if there was a reason for it or not.

On that note, hapi uses this (the socket) to set their internal processing flag and since our stream reference isn't accessible after 'finish', it throws. Should we maybe store a reference to the session directly and not null it on finish? It could lead to some garbage collection issues but on the flipside it's the only issue I found with hapi compatibility.

@apapirovski I don't think what HAPI is actually doing in HTTP1 could be ported 1-1 to HTTP2. I've added this to be compatibile but for all intent and purposes this should not be used: the socket is controlled by the C++/C layers.

@jasnell what do you think?

Yeah, it's a bit weird because they don't actually seem to use the socket based on my quick review of the code — maybe they did in the past. Now they just store a flag on it. When I built a version that retained the socket reference then my test site worked fine with h2 on hapi. But I agree with you re: not using it.

I guess it's all about how much we're willing to compromise for compatibility.

Just a thought but if we do something like process.nextTick(() => (this[kStream] = undefined)); then we get the best of both worlds. It passes all our tests, makes hapi work and gets rid of the stream reference after the user finishes processing the 'finish' event. That's probably the most h1 compatible solution.

mcollina · 2017-09-08T05:03:25Z

test/parallel/test-http2-compat-serverrequest-headers.js

@@ -23,7 +23,6 @@ server.listen(0, common.mustCall(function() {

    assert.strictEqual(request.method, expected[':method']);
    assert.strictEqual(request.scheme, expected[':scheme']);
-    assert.strictEqual(request.path, expected[':path']);


I think we should add an assertion that request.path is undefined for express compatibility.

Good thinking, will update.

apapirovski · 2017-09-08T14:57:08Z

Ok, new commit added. Mostly just addressed feedback and added a couple of extra protections that were missing. The one debatable change is, as I mentioned above, setting kStream to undefined on nextTick instead of immediately. Definitely open to feedback on that.

Also still need to sort out the header validation situation. I might just move that out of this PR since it's not so obvious where we want to validate (people using compatibility API would expect it to validate right away — to match h1 — but we also want to validate in the core which happens later on).

apapirovski · 2017-09-10T14:37:25Z

Moved the header validation out for now. Can revisit in a separate PR.

mcollina · 2017-09-12T07:45:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10041/

mcollina · 2017-09-12T09:04:26Z

This does not land cleanly on master, would you mind rebasing and squashing?

Remove unnecessary variable assignments, remove unreachable code pathways, remove path getter and setter, and other very minor cleanup. Only remove reference to stream on nextTick so that users and libraries can access it in the finish event. Fixes: nodejs#15313 Refs: expressjs/express#3390

apapirovski · 2017-09-12T12:04:00Z

Rebased and squashed, also updated the commit message to be more descriptive + added links. Should be good to go.

mcollina · 2017-09-12T14:13:41Z

Landed as c981483

Remove unnecessary variable assignments, remove unreachable code pathways, remove path getter and setter, and other very minor cleanup. Only remove reference to stream on nextTick so that users and libraries can access it in the finish event. Fixes: #15313 Refs: expressjs/express#3390 PR-URL: #15254 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unnecessary variable assignments, remove unreachable code pathways, remove path getter and setter, and other very minor cleanup. Only remove reference to stream on nextTick so that users and libraries can access it in the finish event. Fixes: nodejs#15313 Refs: expressjs/express#3390 PR-URL: nodejs#15254 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unnecessary variable assignments, remove unreachable code pathways, remove path getter and setter, and other very minor cleanup. Only remove reference to stream on nextTick so that users and libraries can access it in the finish event. Fixes: #15313 Refs: expressjs/express#3390 PR-URL: #15254 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

apapirovski force-pushed the patch-http2-compat-cleanup branch from 4a28942 to 3765d47 Compare September 8, 2017 01:22

mcollina reviewed Sep 8, 2017

View reviewed changes

apapirovski force-pushed the patch-http2-compat-cleanup branch from 94afd5c to a4702dc Compare September 8, 2017 15:17

apapirovski mentioned this pull request Sep 10, 2017

http2 - compat "socket" #15313

Closed

apapirovski force-pushed the patch-http2-compat-cleanup branch from c497173 to 04e2da9 Compare September 11, 2017 03:10

jasnell approved these changes Sep 12, 2017

View reviewed changes

apapirovski force-pushed the patch-http2-compat-cleanup branch from 04e2da9 to 57ab2dc Compare September 12, 2017 11:40

mcollina closed this Sep 12, 2017

apapirovski deleted the patch-http2-compat-cleanup branch September 15, 2017 17:39

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: minor cleanup of compat, tests #15254

http2: minor cleanup of compat, tests #15254

apapirovski commented Sep 8, 2017 •

edited

Loading

mcollina left a comment

mcollina Sep 8, 2017

apapirovski Sep 8, 2017

mcollina Sep 8, 2017

apapirovski Sep 8, 2017 •

edited

Loading

mcollina Sep 8, 2017

mcollina Sep 8, 2017

apapirovski Sep 8, 2017 •

edited

Loading

mcollina Sep 8, 2017

apapirovski Sep 8, 2017

apapirovski Sep 8, 2017

mcollina Sep 8, 2017

apapirovski Sep 8, 2017

apapirovski commented Sep 8, 2017 •

edited

Loading

apapirovski commented Sep 10, 2017

mcollina commented Sep 12, 2017

mcollina commented Sep 12, 2017

apapirovski commented Sep 12, 2017

mcollina commented Sep 12, 2017

http2: minor cleanup of compat, tests #15254

http2: minor cleanup of compat, tests #15254

Conversation

apapirovski commented Sep 8, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mcollina left a 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 Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Sep 8, 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

apapirovski commented Sep 8, 2017 • edited Loading

apapirovski commented Sep 10, 2017

mcollina commented Sep 12, 2017

mcollina commented Sep 12, 2017

apapirovski commented Sep 12, 2017

mcollina commented Sep 12, 2017

apapirovski commented Sep 8, 2017 •

edited

Loading

apapirovski Sep 8, 2017 •

edited

Loading

apapirovski Sep 8, 2017 •

edited

Loading

apapirovski commented Sep 8, 2017 •

edited

Loading