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.

Sign up for GitHub

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

Jump to bottom

http: fix event listener leak #29245

Closed
wants to merge 2 commits into from
Closed

http: fix event listener leak #29245

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 21, 2019 •
edited
Loading

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 21, 2019
@ronag ronag mentioned this pull request Aug 21, 2019
Closed
2 tasks
@ronag ronag force-pushed the http-fix-listener branch 3 times, most recently from 398333c to 40ff776 Compare August 21, 2019 13:18
@ronag ronag mentioned this pull request Aug 21, 2019
Closed
@addaleax
Copy link
Member

Do you think you could add a regression test?

@ronag
Copy link
Member Author

ronag commented Aug 21, 2019

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

@ronag
Copy link
Member Author

ronag commented Aug 21, 2019

@addaleax: added test

@ronag ronag force-pushed the http-fix-listener branch 5 times, most recently from b2d9a28 to 06cceac Compare August 21, 2019 17:27
@ronag ronag force-pushed the http-fix-listener branch from 06cceac to e8c77b1 Compare August 21, 2019 18:37
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ronag @Trott
Update test/parallel/test-http-agent-keepalive.js
6c9a53b 
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag mentioned this pull request Aug 21, 2019
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2019
@Trott
Copy link
Member

Trott commented Aug 23, 2019

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

mcollina
mcollina approved these changes Aug 23, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@ronag
Copy link
Member Author

ronag commented Aug 23, 2019 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Member

Trott 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 pushed a commit to Trott/io.js that referenced this pull request Aug 23, 2019
@Trott
http: fix event listener leak
f39ad8a 
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
Copy link
Member

Trott commented Aug 23, 2019

Landed in f39ad8a

@Trott Trott closed this Aug 23, 2019
@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 23, 2019
@Trott
Copy link
Member

Trott 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 pushed a commit that referenced this pull request Aug 26, 2019
@targos
http: fix event listener leak
dedbd11 
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
@targos
2019-08-26, Version 12.9.1 (Current)
0c2b98f 
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 mentioned this pull request Aug 26, 2019
Merged
targos added a commit that referenced this pull request Aug 26, 2019
@targos
2019-08-26, Version 12.9.1 (Current)
9cfb384 
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
@targos
2019-08-26, Version 12.9.1 (Current)
d770b8a 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@gengjiawen gengjiawen gengjiawen approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ronag @addaleax @nodejs-github-bot @Trott @silverwind @mcollina @jasnell @lpinca @gengjiawen