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: fix event listener leak #29245

Closed
wants to merge 2 commits into from

Conversation

@ronag
Copy link
Contributor

commented Aug 21, 2019

Alternative to #29244

'drain' needs to always to be removed together with 'data' and 'end' when detaching the socket.

Haven't had time to do a unit test yet.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@ronag ronag referenced this pull request Aug 21, 2019
2 of 2 tasks complete

@ronag ronag force-pushed the nxtedition:http-fix-listener branch 3 times, most recently from 398333c to 40ff776 Aug 21, 2019

@addaleax

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Do you think you could add a regression test?

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@addaleax: absolutely, I’ll try to solve that tonight

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@addaleax: added test

@ronag ronag force-pushed the nxtedition:http-fix-listener branch 5 times, most recently from b2d9a28 to 06cceac Aug 21, 2019

@ronag ronag force-pushed the nxtedition:http-fix-listener branch from 06cceac to e8c77b1 Aug 21, 2019

@lpinca
lpinca approved these changes Aug 21, 2019
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Update test/parallel/test-http-agent-keepalive.js
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-bot

This comment has been minimized.

@ronag ronag referenced this pull request Aug 21, 2019
4 of 4 tasks complete
@nodejs-github-bot

This comment has been minimized.

@Trott Trott added the author ready label Aug 23, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Note for whoever lands this: Add Fixes: https://github.com/nodejs/node/issues/29239 to the metadata.

@mcollina
Copy link
Member

left a comment

LGTM

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

I'd like to double check this resolves the warning issue (#29239). However, I'm unsure how to make yarn or npm use the node binary I build? I think @isaacs already checked. But I'd like to know how to do it myself.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

I'm unsure how to make yarn or npm use the node binary I build

IIRC, it's just a PATH lookup. Try export PATH="$PWD:$PATH" while in the directory containing node.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

I'd like to double check this resolves the warning issue (#29239). However, I'm unsure how to make yarn or npm use the node binary I build? I think @isaacs already checked. But I'd like to know how to do it myself.

I used this:

./node `which npm` install -g node-core-utils

This seems to fix the problem. Without this patch, I get the warnings. With it, I do not.

Trott added a commit to Trott/io.js that referenced this pull request Aug 23, 2019
http: fix event listener leak
Fixes: nodejs#29239
PR-URL: nodejs#29245
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Landed in f39ad8a

@Trott Trott closed this Aug 23, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Marked this as notable because we should get this out in a 12.9.1 release and highlight that it fixes the bug in #29239.

targos added a commit that referenced this pull request Aug 26, 2019
http: fix event listener leak
Fixes: #29239
PR-URL: #29245
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit that referenced this pull request Aug 26, 2019
2019-08-26, Version 12.9.1 (Current)
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  #29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  #29263

PR-URL: #29321
@targos targos referenced this pull request Aug 26, 2019
targos added a commit that referenced this pull request Aug 26, 2019
2019-08-26, Version 12.9.1 (Current)
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  #29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  #29263

PR-URL: #29321
targos added a commit that referenced this pull request Aug 26, 2019
2019-08-26, Version 12.9.1 (Current)
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  #29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  #29263

PR-URL: #29321
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http: fix event listener leak
Fixes: nodejs#29239
PR-URL: nodejs#29245
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
2019-08-26, Version 12.9.1 (Current)
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  nodejs#29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  nodejs#29263

PR-URL: nodejs#29321
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http: fix event listener leak
Fixes: nodejs#29239
PR-URL: nodejs#29245
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
2019-08-26, Version 12.9.1 (Current)
Notable changes:

This release fixes two regressions in the http module:

* Fixes an event listener leak in the HTTP client. This resulted in lots
  of warnings during npm/yarn installs.
  nodejs#29245
* Fixes a regression preventing the `'end'` event from being emitted for
  keepalive requests in case the full body was not parsed.
  nodejs#29263

PR-URL: nodejs#29321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.