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.

Sign up for GitHub

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

Jump to bottom

http2: compat avoid bind and properly cleanup stream #20374

Closed
wants to merge 1 commit into from
Closed

http2: compat avoid bind and properly cleanup stream #20374

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 27, 2018 •
edited
Loading

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Apr 27, 2018
@ronag ronag force-pushed the http2-compat-perf branch 4 times, most recently from ebc8bb2 to 34ad49a Compare April 27, 2018 23:02
@ronag ronag changed the title http2: compat avoid bind and propertly cleanup kProxySocket http2: compat avoid bind and properly cleanup stream Apr 27, 2018
@ronag ronag force-pushed the http2-compat-perf branch 3 times, most recently from d57ba33 to 6c86160 Compare April 27, 2018 23:11
mscdex
mscdex reviewed Apr 27, 2018
View reviewed changes
lib/internal/http2/compat.js Outdated
}

function onStreamTrailersReady() {
this[kStream].sendTrailers(this[kTrailers]);
const stream = this;
const res = stream[kResponse];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these extra variables instead of just:

this.sendTrailers(this[kResponse][kTrailers]);

?

Copy link
Member Author

@ronag ronag Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

mscdex
mscdex reviewed Apr 27, 2018
View reviewed changes
lib/internal/http2/compat.js Outdated
}

function onStreamTrailersReady() {
this[kStream].sendTrailers(this[kTrailers]);
stream.sendTrailers(this[kResponse][kTrailers]);
Copy link
Contributor

Choose a reason for hiding this comment

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

stream should be this

apapirovski
apapirovski reviewed Apr 28, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski 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 on a good track but needs a bit of cleanup.

lib/internal/http2/compat.js Outdated
@@ -223,6 +222,31 @@ const proxySocketHandler = {
}
};

function onRequestFinish() {
Copy link
Member

Choose a reason for hiding this comment

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

onStreamCloseRequest? It's not really onRequestFinish.

lib/internal/http2/compat.js Outdated
@@ -223,6 +222,31 @@ const proxySocketHandler = {
}
};

function onRequestFinish() {
const stream = this;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't reassign this.

lib/internal/http2/compat.js Outdated

stream[kProxySocket] = null;
stream[kRequest] = undefined;
stream.off('data', onStreamData);
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/internal/http2/compat.js Outdated
}

function onResponseFinish() {
const stream = this;
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/internal/http2/compat.js Outdated

stream[kProxySocket] = null;
stream[kResponse] = undefined;
stream.off('drain', onStreamDrain);
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/internal/http2/compat.js Outdated
@@ -382,13 +414,14 @@ class Http2ServerResponse extends Stream {
this[kHeaders] = Object.create(null);
this[kTrailers] = Object.create(null);
this[kStream] = stream;
this.writable = true;
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/internal/http2/compat.js Outdated
@@ -236,6 +260,9 @@ class Http2ServerRequest extends Readable {
this[kRawTrailers] = [];
this[kStream] = stream;
this[kAborted] = false;
this.on('pause', onRequestPause);
this.on('resume', onRequestResume);
Copy link
Member

Choose a reason for hiding this comment

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

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

@ronag ronag force-pushed the http2-compat-perf branch 2 times, most recently from d7a0fbd to 525429a Compare April 28, 2018 11:47
@ronag
Copy link
Member Author

ronag commented Apr 28, 2018

@apapirovski cleaned up

@ronag ronag force-pushed the http2-compat-perf branch 2 times, most recently from 20d1dce to 93c4f7d Compare April 28, 2018 11:49
apapirovski
apapirovski reviewed Apr 28, 2018
View reviewed changes
lib/internal/http2/compat.js Outdated
const res = this[kResponse];
const state = res[kState];

if (!this[kResponse] || this.headRequest !== state.headRequest)
Copy link
Member

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
apapirovski reviewed Apr 28, 2018
View reviewed changes
lib/internal/http2/compat.js Outdated
const req = this[kRequest];
const state = req[kState];

if (!this[kRequest])
Copy link
Member

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.

apapirovski
apapirovski approved these changes Apr 28, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@apapirovski
Copy link
Member

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

@BridgeAR 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
@apapirovski
Copy link
Member

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

@mcollina
Copy link
Member

mcollina commented May 7, 2018

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

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

mcollina commented May 7, 2018

👍 to exporting in the internal module.

@apapirovski
Copy link
Member

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.

apapirovski pushed a commit that referenced this pull request May 16, 2018
@ronag @apapirovski
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
Copy link
Member

Landed in 0d762af

MylesBorins pushed a commit that referenced this pull request May 22, 2018
@ronag @MylesBorins
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 addaleax mentioned this pull request May 22, 2018
Merged
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
@ronag @kjin
http2: avoid bind and properly clean up in compat
101361c 
PR-URL: nodejs#20374
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 11, 2018
@ronag @kjin
http2: avoid bind and properly clean up in compat
fce762e 
PR-URL: nodejs#20374
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 19, 2018
@ronag @kjin
http2: avoid bind and properly clean up in compat
6f5b38f 
PR-URL: nodejs#20374
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
@ronag @kjin
http2: avoid bind and properly clean up in compat
11f194f 
PR-URL: nodejs#20374
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@ronag @BethGriggs
http2: avoid bind and properly clean up in compat
d1b7825 
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]>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@trivikr trivikr trivikr approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ronag @apapirovski @mcollina @mscdex @jasnell @trivikr @BridgeAR @nodejs-github-bot