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

http: Eliminate ClientRequest capture by Agent socket listeners #10134

Closed
wants to merge 4 commits into from

Conversation

evantorrie
Copy link
Contributor

@evantorrie evantorrie commented Dec 6, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http

Description of change

Eliminate capture of createSocket callback in onFree/onClose/onRemote listeners by moving them to a separate function.

Fixes #10133

This reduces the heap usage by eliminating the capture in a prior context of the ClientRequest object associated with the first call that opens a socket. Let me know the best way to provide tests for this, as it's non-obvious to me how to do so.

I have provided a heapsnapshot screen shot in the accompanying issue (#10133), and will attach a similar heapsnapshot screen shot showing usage after this fix is included.
heap_noretainer

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 6, 2016
cb(null, s);
}
};

Agent.prototype._installListeners = function installListeners(s, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do this without attaching to the prototype. In its current form, this will become unofficial API that we'll have to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to just a file level function.

@evantorrie
Copy link
Contributor Author

evantorrie commented Dec 6, 2016

@cjihrig Changed per review

cb(null, s);
}
};

function installListeners(agent, s, options) {
var self = agent;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to replace self with agent now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@evantorrie
Copy link
Contributor Author

@cjihrig Changes made as requested.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

cb(null, s);
}
};

function installListeners(agent, s, options) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this blank line.

@evantorrie
Copy link
Contributor Author

Removed the blank line. @cjihrig

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but can you make sure the commit log conforms to the style guide from CONTRIBUTING.md?

@evantorrie evantorrie changed the title Eliminate capture of "cb" in createSocket context http: Eliminate capture of "cb" by Agent socket listeners Dec 9, 2016
@evantorrie evantorrie changed the title http: Eliminate capture of "cb" by Agent socket listeners http: Eliminate ClientRequest capture by Agent socket listeners Dec 9, 2016
@evantorrie
Copy link
Contributor Author

@bnoordhuis Does that look better? Do you want me to rebase/squash? Or is that something you do on your end?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 9, 2016

The only CI failure was the linter. Does make lint pass for you?

Move the onFree/onClose/onRemote listeners to a separate function
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.
@evantorrie
Copy link
Contributor Author

@cjihrig Yes, works for me:

[root@98dc4c605317 node]# git log --oneline -5 --decorate
518d33d (HEAD, agent-retainer) http: eliminate capture of ClientRequest in Agent
eb5b0d3 change self to agent
f774e72 change to local function instead of prototype
3967101 Eliminate capture of "cb" in createSocket context
bc335c0 (upstream/master) doc: buffer allocation throws for negative size
[root@98dc4c605317 node]# scl enable devtoolset-4 python27 -- gmake lint
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools
Total errors found: 0
[root@98dc4c605317 node]#

@evantorrie
Copy link
Contributor Author

@cjihrig Is there anything more I need to do here?

@bnoordhuis
Copy link
Member

@evantorrie
Copy link
Contributor Author

test/arm and test/smartos look like flaky failures unrelated to this change.

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

LGTM. Running CI one more time to try and get a green build: https://ci.nodejs.org/job/node-test-pull-request/5418/

Thanks!

@evantorrie
Copy link
Contributor Author

These CI failures still look unrelated to this change - are the particular failing tests known to be flaky?

@evantorrie
Copy link
Contributor Author

@evanlucas CI failure seems to be unrelated to this change.

jasnell pushed a commit that referenced this pull request Jan 6, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

PR-URL: #10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

Landed in c04d4df

@jasnell jasnell closed this Jan 6, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

PR-URL: nodejs#10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

PR-URL: nodejs#10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

PR-URL: nodejs#10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

PR-URL: nodejs#10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

MylesBorins pushed a commit that referenced this pull request Sep 22, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

Backport-PR-URL: #15500
PR-URL: #10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 22, 2017
MylesBorins pushed a commit that referenced this pull request Sep 26, 2017
Keepalive sockets that are returned to the agent's freesocket pool were
previously capturing a reference to the ClientRequest that initiated the
request.

This commit eliminates that by moving the installation of the socket
listeners to a different function.

Backport-PR-URL: #15500
PR-URL: #10134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
10 participants