http: remove stale timeout listeners #9440

karlbohlmark · 2016-11-03T11:45:10Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http

Description of change

In order to prevent a memory leak when using keep alive, ensure that the
timeout listener for the request is removed when the response has ended.

This fixes #9268

In order to prevent a memory leak when using keep alive, ensure that the timeout listener for the request is removed when the response has ended.

evanlucas · 2016-11-03T12:44:38Z

lib/_http_client.js

-    socket.once('timeout', () => req.emit('timeout'));
+    const emitRequestTimeout = () => req.emit('timeout');
+    socket.once('timeout', emitRequestTimeout);
+    req.on('response', (r) => {


hm, I wonder if the response event is emitted in all cases if the request is aborted...will have to dig into that

No, I don't think it is, but I think the socket is destroyed, which solves the cleanup issue in that case.

jasnell · 2016-11-03T15:14:42Z

test/parallel/test-http-client-timeout-option-listeners.js

+      agent.destroy();
+    })
+    .catch((e) => {
+      console.log(e);


Unless the console.log() is part of the test, I'd prefer to omit the extraneous output

Perhaps instead make this a common.fail()?

I could remove the whole catch clause and add a

process.on('unhandledRejection', function () { common.fail('A promise rejection was unhandled') });

Would that be ok?

I want to ensure that the test never hangs

grahamj · 2016-11-03T15:26:01Z

I usually lurk so excuse if I'm being too picky... The patch looks good but perhaps there should there be a test to confirm that this actually fixes the leak? It's possible that something else is also contributing, such as @evanlucas 's concern about end always being emitted or possibly other listeners similarly not being removed.

Just a thought, thanks for the patch!

plemarquand · 2016-11-03T18:29:14Z

I just tried this patch locally and it does indeed fix the memory leak we were seeing. 👍

cjihrig · 2016-11-03T23:27:05Z

test/parallel/test-http-client-timeout-option-listeners.js

+  agent,
+  method: 'GET',
+  port: undefined,
+  host: '127.0.0.1',


Instead of 127.0.0.1 throughout the test, can you use common.localhostIPv4.

karlbohlmark · 2016-11-10T15:33:21Z

@evanlucas Is there anything more I can do on this? Rebase and squash?

We are currently not able to use the latest LTS (or the latest Current) because of this memory leak. It would be really nice to get any fix in for this.

Trott · 2016-11-10T19:59:15Z

/cc @nodejs/http

mjsalinger · 2016-11-16T14:43:26Z

This is definitely affecting our ability to use the latest LTS release...

evanlucas · 2016-11-18T11:27:35Z

Sorry for the delay. I'm still digging in to make sure we don't need to clean up anything else related to the timeout. In the meantime, CI: https://ci.nodejs.org/job/node-test-pull-request/4883/

evanlucas · 2016-11-18T14:14:22Z

Looks like there is a related test failure on arm. (https://ci.nodejs.org/job/node-test-binary-arm/4752/RUN_SUBSET=3,label=pi1-raspbian-wheezy/tapResults/)

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: A promise rejection was unhandled
    at Object.exports.fail (/home/iojs/build/workspace/node-test-binary-arm/test/common.js:443:10)
    at process.<anonymous> (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-http-client-timeout-option-listeners.js:22:10)
    at emitTwo (events.js:106:13)
    at process.emit (events.js:191:7)
    at emitPendingUnhandledRejections (internal/process/promises.js:72:22)
    at process._tickCallback (internal/process/next_tick.js:104:7)

cjihrig · 2016-11-18T16:10:23Z

test/parallel/test-http-client-timeout-option-listeners.js

+  port: undefined,
+  host: common.localhostIPv4,
+  path: '/',
+  timeout: 100


Can you wrap the timeout in common.platformTimeout()

evanlucas · 2016-11-18T16:24:23Z

test/parallel/test-http-client-timeout-option-listeners.js

+    http.request(options, (response) => {
+      const sockets = agent.sockets[`${options.host}:${options.port}:`];
+      if (sockets.length !== 1) {
+        reject(new Error('Only one socket should be created'));


Should there be a return here?

evanlucas · 2016-11-21T11:18:28Z

Trying CI again. https://ci.nodejs.org/job/node-test-pull-request/4918/

karlbohlmark · 2016-11-22T18:47:22Z

@evanlucas That failure doesn't seem related, right?

evanlucas · 2016-11-23T13:43:57Z

@karlbohlmark ah I restarted CI, so I'm not sure which one it was. I'm still doing some additional testing around this to make sure it doesn't affect anything else. We should be able to get this in soon though. Thanks for being patient!

evanlucas · 2016-11-25T13:01:44Z

/cc @nodejs/http PTAL

bnoordhuis · 2016-11-25T15:23:17Z

lib/_http_client.js

-    socket.once('timeout', () => req.emit('timeout'));
+    const emitRequestTimeout = () => req.emit('timeout');
+    socket.once('timeout', emitRequestTimeout);
+    req.on('response', (r) => {


Can you call the argument res for consistency/parity? Is there a reason to use .on instead of .once? The timeout listener is installed with .once().

bnoordhuis · 2016-11-25T15:28:35Z

test/parallel/test-http-client-timeout-option-listeners.js

+      response.on('end', () => resolve(numListeners));
+    }).end();
+  });
+}


I'd write the test in simple callback style. Less chance of accidentally swallowing errors and easier to debug because you can simply assert.strictEqual(sockets.length, 1) instead of calling reject().

italoacasas · 2016-12-08T21:09:41Z

Landed 9a5d475

Thanks for the contribution.

In order to prevent a memory leak when using keep alive, ensure that the timeout listener for the request is removed when the response has ended. PR-URL: #9440 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2.fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 PR-URL: #10277

Notable changes SEMVER-MINOR - url: - add inspect function to TupleOrigin (Safia Abdalla) #10039 - crypto: - allow adding extra certs to well-known CAs (Sam Roberts) #9139 SEMVER-PATCH - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 - fs: - remove unused argument from copyObject() (Ethan Arrowood) #10041 - timers: - fix handling of cleared immediates (hveldstra) #9759 PR-URL: #10277

Notable changes: * **crypto**: - Allow adding extra certificates to well-known CAs. (Sam Roberts) [#9139](#9139) * **buffer**: - Fix single-character string filling. (Anna Henningsen) [#9837](#9837) - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen) [#9837](#9837) * **url**: - Add inspect function to TupleOrigin. (Safia Abdalla) [#10039](#10039) - Including base argument in originFor. (joyeecheung) [#10021](#10021) - Improve URLSearchParams spec compliance. (Timothy Gu) [#9484](#9484) * **http**: - Remove stale timeout listeners. (Karl Böhlmark) [#9440](#9440) * **build**: - Fix node_g target. (Daniel Bevenius) [#10153](#10153) * **fs**: - Remove unused argument from copyObject(). (EthanArrowood) [#10041](#10041) * **timers**: - Fix handling of cleared immediates. (hveldstra) [#9759](#9759) * **src**: - Add wrapper for process.emitWarning(). (SamRoberts) [#9139](#9139) - Fix string format mistake for 32 bit node.(Alex Newman) [#10082](#10082)

Notable changes: * **crypto**: The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) [#9139](#9139) * **buffer**: buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) [#9837](#9837) * **url**: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) [#10021](#10021) - Improve URLSearchParams to meet specification compliance. (Timothy Gu) [#9484](#9484) * **http**: Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) [#9440](#9440)

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) nodejs#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs#9484 PR-URL: nodejs#10277

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) #9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) #9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) #9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) #10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) #9484 PR-URL: #10277

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs/node#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs/node#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Bohlmark) nodejs/node#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs/node#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs/node#9484 PR-URL: nodejs/node#10277 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

In order to prevent a memory leak when using keep alive, ensure that the timeout listener for the request is removed when the response has ended. PR-URL: #9440 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-01-16T16:22:43Z

I've landed this in v6.x-staging. I do not believe this needs to be included in the v4.x release line. Please let me know if I am mistakedn

sindreij · 2017-01-19T08:46:51Z

When will this land in 6.x?

gibfahn · 2017-01-19T08:56:33Z

@sindreij

I've landed this in v6.x-staging.

This means it should be in the next v6.x release, which is planned for February 21st (not a firm date, see this issue).

In order to prevent a memory leak when using keep alive, ensure that the timeout listener for the request is removed when the response has ended. PR-URL: #9440 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

http: remove stale timeout listeners

65790af

In order to prevent a memory leak when using keep alive, ensure that the timeout listener for the request is removed when the response has ended.

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

evanlucas reviewed Nov 3, 2016

View reviewed changes

jasnell reviewed Nov 3, 2016

View reviewed changes

replace catch with unhandledRejection-listener

5c37b4a

cjihrig reviewed Nov 3, 2016

View reviewed changes

use common.localhostIPv4

274cb28

Trott added the lts-watch-v6.x label Nov 16, 2016

sam-github mentioned this pull request Nov 18, 2016

HTTPParser inhibits garbage collection on keep-alive connections #9668

Closed

Increase timeout, output rejection reason

c32ee77

cjihrig reviewed Nov 18, 2016

View reviewed changes

use common.platformTimeout

2c4ebae

evanlucas suggested changes Nov 18, 2016

View reviewed changes

early exit on rejection

fa6ec18

andrasq mentioned this pull request Nov 18, 2016

Response strings and closures objects leak with http.Agent enabled (Memory Leak) #9530

Closed

evanlucas approved these changes Nov 25, 2016

View reviewed changes

bnoordhuis requested changes Nov 25, 2016

View reviewed changes

Karl Böhlmark added 2 commits November 25, 2016 16:40

use EventEmitter#once

516ba6f

test in callback-style

60ff0fb

italoacasas closed this Dec 8, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

targos mentioned this pull request Jan 8, 2017

Making several http requests using keep-alive mode generates this trace: Warning: Possible EventEmitter memory leak detected. #9268

Closed

MylesBorins added dont-land-on-v4.x and removed lts-watch-v6.x labels Jan 16, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

mihai-iorga mentioned this pull request Feb 9, 2017

Possible memory leak? node-apn/node-apn#518

Open

aleung mentioned this pull request Mar 24, 2017

http.request timeout doesn't take effect #12005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: remove stale timeout listeners #9440

http: remove stale timeout listeners #9440

karlbohlmark commented Nov 3, 2016

evanlucas Nov 3, 2016

karlbohlmark Nov 3, 2016 •

edited

Loading

jasnell Nov 3, 2016

jasnell Nov 3, 2016

karlbohlmark Nov 3, 2016

grahamj commented Nov 3, 2016

plemarquand commented Nov 3, 2016

cjihrig Nov 3, 2016

karlbohlmark commented Nov 10, 2016

Trott commented Nov 10, 2016

mjsalinger commented Nov 16, 2016

evanlucas commented Nov 18, 2016

evanlucas commented Nov 18, 2016

cjihrig Nov 18, 2016

evanlucas Nov 18, 2016

evanlucas commented Nov 21, 2016

karlbohlmark commented Nov 22, 2016

evanlucas commented Nov 23, 2016

evanlucas commented Nov 25, 2016

bnoordhuis Nov 25, 2016

bnoordhuis Nov 25, 2016

italoacasas commented Dec 8, 2016

MylesBorins commented Jan 16, 2017

sindreij commented Jan 19, 2017

gibfahn commented Jan 19, 2017

http: remove stale timeout listeners #9440

http: remove stale timeout listeners #9440

Conversation

karlbohlmark commented Nov 3, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

karlbohlmark Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamj commented Nov 3, 2016

plemarquand commented Nov 3, 2016

Choose a reason for hiding this comment

karlbohlmark commented Nov 10, 2016

Trott commented Nov 10, 2016

mjsalinger commented Nov 16, 2016

evanlucas commented Nov 18, 2016

evanlucas commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Nov 21, 2016

karlbohlmark commented Nov 22, 2016

evanlucas commented Nov 23, 2016

evanlucas commented Nov 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

italoacasas commented Dec 8, 2016

MylesBorins commented Jan 16, 2017

sindreij commented Jan 19, 2017

gibfahn commented Jan 19, 2017

karlbohlmark Nov 3, 2016 •

edited

Loading