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

stream: do cleanup when iterator is destroyed #42320

Merged
merged 3 commits into from
Mar 16, 2022
Merged

stream: do cleanup when iterator is destroyed #42320

merged 3 commits into from
Mar 16, 2022

Conversation

KhooHaoYit
Copy link
Contributor

When you create a readable.iterator from a stream with options.destroyOnReturn on false, the iterator doesn't do any cleanup when the iterator exits, causing it to add multiple listeners to the stream

Code that reproduces the bug

const pass = new stream.PassThrough().end();
for(let amount = 15; amount; --amount){
  for await (const chunk of pass.iterator({ destroyOnReturn: false })){}
}

Console output

(node:20524) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 readable listeners added to [PassThrough]. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)
(node:20524) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 end listeners added to [PassThrough]. Use emitter.setMaxListeners() to increase limit
(node:20524) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 finish listeners added to [PassThrough]. Use emitter.setMaxListeners() to increase limit
(node:20524) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [PassThrough]. Use emitter.setMaxListeners() to increase limit
(node:20524) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [PassThrough]. Use emitter.setMaxListeners() to increase limit

Tested node version

v17.6.0 on Windows 10 and v18.0.0-pre @ b481beecd8

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 13, 2022
@ronag ronag added stream Issues and PRs related to the stream subsystem. request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Mar 13, 2022
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.

Can you please add a unit test?

@mcollina mcollina removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2022
@nodejs-github-bot
Copy link
Collaborator

@KhooHaoYit KhooHaoYit requested a review from mcollina March 13, 2022 13:19
@benjamingr
Copy link
Member

@Linkgoron

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 15, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42320
✔  Done loading data for nodejs/node/pull/42320
----------------------------------- PR info ------------------------------------
Title      stream: do cleanup when iterator is destroyed (#42320)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     KhooHaoYit:stream-iterator-fix -> nodejs:master
Labels     stream, commit-queue-squash
Commits    3
 - stream: do cleanup when iterator is destroyed
 - test: check for dangling listeners
 - test: enable no-unused-vars ESLint rule
Committers 1
 - Khoo Hao Yit <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/42320
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Nitzan Uziely 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42320
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Nitzan Uziely 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 13 Mar 2022 11:59:44 GMT
   ✔  Approvals: 5
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/42320#pullrequestreview-908129087
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/42320#pullrequestreview-910068802
   ✔  - Nitzan Uziely (@linkgoron): https://github.com/nodejs/node/pull/42320#pullrequestreview-908140795
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/42320#pullrequestreview-908141108
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42320#pullrequestreview-908891052
   ✖  This PR needs to wait 15 more minutes to land
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-03-14T17:01:15Z: https://ci.nodejs.org/job/node-test-pull-request/43020/
- Querying data for job/node-test-pull-request/43020/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1986430799

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Mar 15, 2022
@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 16, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 16, 2022
@nodejs-github-bot nodejs-github-bot merged commit 6d3920d into nodejs:master Mar 16, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 6d3920d

@KhooHaoYit KhooHaoYit deleted the stream-iterator-fix branch March 17, 2022 00:40
bengl pushed a commit that referenced this pull request Mar 21, 2022
PR-URL: #42320
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bengl bengl mentioned this pull request Mar 21, 2022
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
PR-URL: nodejs#42320
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants