http2: compat avoid bind and properly cleanup stream#20374

ronag · 2018-04-27T22:36:50Z

Avoid unnecessary function bind.

Also fixes a bug where kProxySocket was assigned null on the wrong object as well as properly cleanup the stream object.

Checklis

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

mscdex · 2018-04-27T23:26:26Z

lib/internal/http2/compat.js

Why these extra variables instead of just:
this.sendTrailers(this[kResponse][kTrailers]);
?

No particular reason. Just style. I'm fine with changing if you want me to?

mscdex · 2018-04-27T23:38:25Z

lib/internal/http2/compat.js

stream should be this

apapirovski

This is on a good track but needs a bit of cleanup.

apapirovski · 2018-04-28T05:45:27Z

lib/internal/http2/compat.js

onStreamCloseRequest? It's not really onRequestFinish.

apapirovski · 2018-04-28T05:45:43Z

lib/internal/http2/compat.js

Please don't reassign this.

apapirovski · 2018-04-28T05:50:26Z

lib/internal/http2/compat.js

Are all these detachments necessary? It will certainly make performance worse.

apapirovski · 2018-04-28T05:51:31Z

lib/internal/http2/compat.js

Same as above: more accurate name and don't re-assign this please.

apapirovski · 2018-04-28T05:53:16Z

lib/internal/http2/compat.js

Same as above. Unless this is 100% necessary, it would be better not to.

apapirovski · 2018-04-28T05:53:48Z

lib/internal/http2/compat.js

This change doesn't seem to relate to this PR.

apapirovski · 2018-04-28T05:54:16Z

lib/internal/http2/compat.js

This change doesn't seem to relate to this PR.

ronag · 2018-04-28T11:47:44Z

@apapirovski cleaned up

apapirovski · 2018-04-28T14:32:03Z

lib/internal/http2/compat.js

res === undefined would be cleaner check here.

apapirovski · 2018-04-28T14:32:53Z

lib/internal/http2/compat.js

req === undefined is more precise. Also please move const state = after the check.

apapirovski

LGTM thanks!

apapirovski · 2018-04-28T16:28:49Z

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

apapirovski · 2018-05-07T08:28:57Z

ping @nodejs/http2 would be nice to have at least one more review on this.

mcollina · 2018-05-07T09:27:27Z

Also fixes a bug where kProxySocket was assigned null on the wrong object as well as properly cleanup the stream object.

Can you add a test for this?

apapirovski · 2018-05-07T09:31:21Z

FWIW I don't think it's possible to test this without exporting the kProxySocket symbol. Although
since it's in internal, it's probably ok to do so.

mcollina · 2018-05-07T09:33:24Z

👍 to exporting in the internal module.

apapirovski · 2018-05-14T16:37:21Z

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

I will land this once done and create a test myself in a separate PR.

PR-URL: #20374 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

apapirovski · 2018-05-16T04:42:57Z

Landed in 0d762af

PR-URL: #20374 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

PR-URL: nodejs#20374 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Backport-PR-URL: #22850 PR-URL: #20374 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Apr 27, 2018

ronag force-pushed the http2-compat-perf branch 4 times, most recently from ebc8bb2 to 34ad49aCompare April 27, 2018 23:02

ronag changed the title ~~http2: compat avoid bind and propertly cleanup kProxySocket~~http2: compat avoid bind and properly cleanup streamApr 27, 2018

ronag force-pushed the http2-compat-perf branch 3 times, most recently from d57ba33 to 6c86160Compare April 27, 2018 23:11

mscdex reviewed Apr 27, 2018
View reviewed changes

ronag force-pushed the http2-compat-perf branch from 6c86160 to 8a5874aCompare April 27, 2018 23:37

mscdex reviewed Apr 27, 2018
View reviewed changes

lib/internal/http2/compat.js Outdated
Copy link
Contributor
mscdexApr 27, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stream should be this

ronag force-pushed the http2-compat-perf branch from 8a5874a to 96b0de9Compare April 27, 2018 23:40

apapirovski reviewed Apr 28, 2018
View reviewed changes

ronag force-pushed the http2-compat-perf branch 2 times, most recently from d7a0fbd to 525429aCompare April 28, 2018 11:47

ronag force-pushed the http2-compat-perf branch 2 times, most recently from 20d1dce to 93c4f7dCompare April 28, 2018 11:49

apapirovski reviewed Apr 28, 2018
View reviewed changes

lib/internal/http2/compat.js Outdated
Copy link
Contributor
apapirovskiApr 28, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res === undefined would be cleaner check here.

apapirovski reviewed Apr 28, 2018
View reviewed changes

lib/internal/http2/compat.js Outdated
Copy link
Contributor
apapirovskiApr 28, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
req === undefined is more precise. Also please move const state = after the check.

ronag force-pushed the http2-compat-perf branch from 93c4f7d to 823bc93Compare April 28, 2018 14:40

http2: compat avoid bind and propertly cleanup stream.
ae50653

ronag force-pushed the http2-compat-perf branch from 823bc93 to ae50653Compare April 28, 2018 14:41

apapirovski approved these changes Apr 28, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2018

jasnell approved these changes May 7, 2018
View reviewed changes

trivikr approved these changes May 14, 2018
View reviewed changes

apapirovski pushed a commit that referenced this pull request May 16, 2018
http2: avoid bind and properly clean up in compat
0d762af
PR-URL: #20374 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

apapirovski closed this May 16, 2018

MylesBorins pushed a commit that referenced this pull request May 22, 2018
http2: avoid bind and properly clean up in compat
de2b047
PR-URL: #20374 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

addaleax mentioned this pull request May 22, 2018
v10.2.0 proposal #20724
Merged

BethGriggs mentioned this pull request Oct 30, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

Uh oh!

http2: compat avoid bind and properly cleanup stream#20374

http2: compat avoid bind and properly cleanup stream #20374

Uh oh!

Conversation

ronag commented Apr 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklis

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronagApr 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag commented Apr 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Apr 28, 2018

Uh oh!

apapirovski commented May 7, 2018

Uh oh!

mcollina commented May 7, 2018

Uh oh!

apapirovski commented May 7, 2018

Uh oh!

mcollina commented May 7, 2018

Uh oh!

apapirovski commented May 14, 2018

Uh oh!

apapirovski commented May 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ronag commented Apr 27, 2018•
edited
Loading

ronagApr 27, 2018•
edited
Loading