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: compat avoid bind and properly cleanup stream #20374

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 27, 2018

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
}

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

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?

}

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

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.

@@ -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.

@@ -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.


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.

}

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.


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.

@@ -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.

@@ -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
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.

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.

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

@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
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
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
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
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
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
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants