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

lib: promise version of streams.finished call cleanup after eos #44862

Merged
merged 4 commits into from
Oct 15, 2022

Conversation

ntedgi
Copy link
Contributor

@ntedgi ntedgi commented Oct 2, 2022

ref: #44556
src: add autoCleanup logic to finished
docs: add autoCleanup true as default

@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 Oct 2, 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.

Thanks for opening a PR! Can you please add a unit test?

doc/api/stream.md Outdated Show resolved Hide resolved
@ntedgi
Copy link
Contributor Author

ntedgi commented Oct 2, 2022

@mcollina @ronag

i added the following tests

{
  const streamObj = new Readable();
  assert.throws(
    () => {
      // Passing autoCleanup option not as boolean
      // should throw error
      finished(streamObj, {autoCleanup: 2});
    },
    {code: 'ERR_INVALID_ARG_TYPE'}
  );
}

// Below code should not throw any errors as the
// streamObj is `Stream` and autoCleanup is boolean
{
  const streamObj = new Readable();
  finished(streamObj, {autoCleanup: true})
}

In addition, I want to make sure that the cleanup function has been called as expected

I didn't find any example of how to detect listeners has been properly removed

I see it hasn't been tested here as well :

https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js

Can you please help me think of a simple way to check
this cases :
cleanup function should be called when autoCleanup is set to true
cleanup function should not be called when autoCleanup is set to false
cleanup function should not be called if no value passed to autoCleanup

@ntedgi
Copy link
Contributor Author

ntedgi commented Oct 2, 2022

@mcollina @ronag

i added the following tests

{
  const streamObj = new Readable();
  assert.throws(
    () => {
      // Passing autoCleanup option not as boolean
      // should throw error
      finished(streamObj, {autoCleanup: 2});
    },
    {code: 'ERR_INVALID_ARG_TYPE'}
  );
}

// Below code should not throw any errors as the
// streamObj is `Stream` and autoCleanup is boolean
{
  const streamObj = new Readable();
  finished(streamObj, {autoCleanup: true})
}

In addition, I want to make sure that the cleanup function has been called as expected

I didn't find any example of how to detect listeners has been properly removed

I see it hasn't been tested here as well :

https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js

Can you please help me think of a simple way to check this cases : cleanup function should be called when autoCleanup is set to true cleanup function should not be called when autoCleanup is set to false cleanup function should not be called if no value passed to autoCleanup

@mcollina @ronag

i added the following tests

{
  const streamObj = new Readable();
  assert.throws(
    () => {
      // Passing autoCleanup option not as boolean
      // should throw error
      finished(streamObj, {autoCleanup: 2});
    },
    {code: 'ERR_INVALID_ARG_TYPE'}
  );
}

// Below code should not throw any errors as the
// streamObj is `Stream` and autoCleanup is boolean
{
  const streamObj = new Readable();
  finished(streamObj, {autoCleanup: true})
}

In addition, I want to make sure that the cleanup function has been called as expected

I didn't find any example of how to detect listeners has been properly removed

I see it hasn't been tested here as well :

https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js

Can you please help me think of a simple way to check this cases : cleanup function should be called when autoCleanup is set to true cleanup function should not be called when autoCleanup is set to false cleanup function should not be called if no value passed to autoCleanup

Thank you anyway, I was able to find a simple solution
just to use the listenerCount

{
  const streamObj = new Writable();
  assert(streamObj.listenerCount('end') === 0);
  finished(streamObj, {autoCleanup: false}).then(() => {
    assert(streamObj.listenerCount('end') === 1);
  })
}

@ntedgi ntedgi changed the title feat: promise version of streams.finished call clean up lib: promise version of streams.finished call clean up Oct 2, 2022
@ntedgi ntedgi force-pushed the main branch 2 times, most recently from 1ba59b1 to 669dc00 Compare October 2, 2022 20:12
implement autoCleanup logic. update docs add autoCleanup description

ref: nodejs#44556
doc: update autoCleanup default value to false
@ntedgi ntedgi changed the title lib: promise version of streams.finished call clean up lib: promise version of streams.finished call cleanup after eos Oct 2, 2022
@ntedgi ntedgi requested review from mcollina and ronag and removed request for mcollina and ronag October 3, 2022 04:23
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2022
@nodejs-github-bot
Copy link
Collaborator

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

{
const streamObj = new Writable();
assert.strictEqual(streamObj.listenerCount('end'), 0);
finished(streamObj, { cleanup: true }).then(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpic: wrap with common.mustCall

{
const streamObj = new Writable();
assert.strictEqual(streamObj.listenerCount('end'), 0);
finished(streamObj).then(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Same

@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 14, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Oct 15, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2022
@nodejs-github-bot nodejs-github-bot merged commit 84064bf into nodejs:main Oct 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 84064bf

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
implement autoCleanup logic. update docs add autoCleanup description

ref: #44556
PR-URL: #44862
Refs: #44556
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
implement autoCleanup logic. update docs add autoCleanup description

ref: #44556
PR-URL: #44862
Refs: #44556
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
implement autoCleanup logic. update docs add autoCleanup description

ref: #44556
PR-URL: #44862
Refs: #44556
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
implement autoCleanup logic. update docs add autoCleanup description

ref: #44556
PR-URL: #44862
Refs: #44556
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
implement autoCleanup logic. update docs add autoCleanup description

ref: #44556
PR-URL: #44862
Refs: #44556
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Sep 25, 2024
PR-URL: #55043
Refs: #44862
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
PR-URL: #55043
Refs: #44862
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants