http: Eliminate ClientRequest capture by Agent socket listeners by evantorrie · Pull Request #10134 · nodejs/node · GitHub

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.

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.

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

I changed it to just a file level function.

@cjihrig Changed per review

You should be able to replace self with agent now.

Done

@cjihrig Changes made as requested.

LGTM with one small nit. CI: https://ci.nodejs.org/job/node-test-pull-request/5309/

Can you remove this blank line.

Removed the blank line. @cjihrig

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

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

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.

@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]#

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

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

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

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

Thanks!

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

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

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>

Landed in c04d4df

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>

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

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>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 6, 2016

cjihrig requested changes Dec 6, 2016

View reviewed changes

cjihrig reviewed Dec 6, 2016

View reviewed changes

cjihrig approved these changes Dec 8, 2016

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Dec 8, 2016

addaleax removed the dont-land-on-v7.x label Dec 8, 2016

bnoordhuis approved these changes Dec 8, 2016

View reviewed changes

evantorrie changed the title Eliminate capture of "cb" in createSocket context http: Eliminate capture of "cb" by Agent socket listeners Dec 9, 2016

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 force-pushed the agent-retainer branch from 054d7b2 to 97f80b9 Compare December 9, 2016 00:54

Evan Torrie added 4 commits December 9, 2016 09:04

Eliminate capture of "cb" in createSocket context

3967101

Move the onFree/onClose/onRemote listeners to a separate function

change to local function instead of prototype

f774e72

change self to agent

eb5b0d3

evantorrie force-pushed the agent-retainer branch from 97f80b9 to 518d33d Compare December 9, 2016 17:10

Fishrock123 added dont-land-on-v7.x and removed dont-land-on-v7.x labels Dec 9, 2016

evanlucas approved these changes Dec 15, 2016

View reviewed changes

jasnell approved these changes Jan 6, 2017

View reviewed changes

jasnell closed this Jan 6, 2017

lance mentioned this pull request Jan 19, 2017

http.Agent free sockets maintain reference to initial ClientRequest object until the socket is destroyed/closed #10133

Closed

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added dont-land-on-v4.x labels Mar 7, 2017

drewfish mentioned this pull request Sep 19, 2017

backport http: Eliminate ClientRequest capture by Agent socket listeners nodejs/Release#247

Closed

lpinca mentioned this pull request Sep 21, 2017

[v6.x backport] http: eliminate capture of ClientRequest in Agent #15500

Closed

4 tasks

gibfahn added backport-requested-v6.x and removed dont-land-on-v6.x labels Sep 22, 2017

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Sep 22, 2017

MylesBorins mentioned this pull request Sep 22, 2017

v6.11.4 proposal #15506

Merged

http: Eliminate ClientRequest capture by Agent socket listeners by evantorrie · Pull Request #10134 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: Eliminate ClientRequest capture by Agent socket listeners - #10134

http: Eliminate ClientRequest capture by Agent socket listeners#10134evantorrie wants to merge 4 commits intonodejs:masternodejs/node:masterfrom evantorrie:agent-retainerevantorrie/node:agent-retainerCopy head branch name to clipboard

Conversation

evantorrie commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrig Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

evantorrie Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

evantorrie commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

evantorrie Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

evantorrie commented Dec 7, 2016

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

evantorrie commented Dec 8, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

evantorrie commented Dec 9, 2016

Uh oh!

cjihrig commented Dec 9, 2016

Uh oh!

evantorrie commented Dec 9, 2016

Uh oh!

evantorrie commented Dec 13, 2016

Uh oh!

bnoordhuis commented Dec 13, 2016

Uh oh!

evantorrie commented Dec 14, 2016

Uh oh!

evanlucas left a comment

Choose a reason for hiding this comment

Uh oh!

evantorrie commented Dec 19, 2016

Uh oh!

evantorrie commented Dec 22, 2016

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

MylesBorins commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

http: Eliminate ClientRequest capture by Agent socket listeners#10134
evantorrie wants to merge 4 commits into
nodejs:masterfrom
evantorrie:agent-retainer

evantorrie commented Dec 6, 2016 •
edited
Loading

evantorrie commented Dec 6, 2016 •
edited
Loading