GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: promise version of `streams.finished` call cleanup after eos by ntedgi · Pull Request #44862 · nodejs/node

Conversation

@ntedgi

@ntedgi ntedgi commented Oct 2, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Oct 2, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 2, 2022
mcollina
mcollina requested changes Oct 2, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ntedgi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ronag
ronag reviewed Oct 2, 2022
View reviewed changes
Comment thread doc/api/stream.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ntedgi

ntedgi commented Oct 2, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ntedgi

ntedgi commented Oct 2, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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);
  })
}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 added 2 commits October 2, 2022 23:14
@ntedgi
lib: promise version of streams.finished call clean up
9b778f1
implement autoCleanup logic. update docs add autoCleanup description

ref: #44556
@ntedgi
test: adding unit tests to cover the autoCleanup parameter logic
cd80cdc
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
fix linting issues
b65461a
@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. Only starts once the PR has an approving review. label Oct 6, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 6, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 6, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 6, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Oct 7, 2022
CI Reliability 2022-10-07 nodejs/reliability#397
Open
CI Reliability 2022-10-08 nodejs/reliability#398
Open
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 14, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 14, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow
MoLow reviewed Oct 14, 2022
View reviewed changes
Comment thread test/parallel/test-stream-promises.js
{
const streamObj = new Writable();
assert.strictEqual(streamObj.listenerCount('end'), 0);
finished(streamObj, { cleanup: true }).then(() => {

@MoLow MoLow Oct 14, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ntedgi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
MoLow
MoLow reviewed Oct 14, 2022
View reviewed changes
Comment thread test/parallel/test-stream-promises.js
{
const streamObj = new Writable();
assert.strictEqual(streamObj.listenerCount('end'), 0);
finished(streamObj).then(() => {

@MoLow MoLow Oct 14, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@MoLow MoLow added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 14, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 15, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 15, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina added commit-queue-squash PRs the Commit Queue should land as one squashed commit. commit-queue PRs queued for automated landing through the Commit Queue. 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 PRs queued for automated landing through the Commit Queue. label Oct 15, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 84064bf into nodejs:main Oct 15, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 15, 2022

Copy link
Copy Markdown
Collaborator

Landed in 84064bf

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Oct 18, 2022
CI Reliability 2022-10-18 nodejs/reliability#403
Open
CI Reliability 2022-10-19 nodejs/reliability#404
Open
@MoLow MoLow mentioned this pull request Oct 19, 2022
test: add missing common.mustCall #45064
Merged
@github-actions github-actions Bot mentioned this pull request Oct 20, 2022
CI Reliability 2022-10-20 nodejs/reliability#405
Open
10 tasks
@targos targos mentioned this pull request Oct 22, 2022
Promise version of streams.finished not calling clean up #44556
Closed
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@ntedgi @RafaelGSS
lib: promise version of streams.finished call clean up
28b6fd5
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
v19.1.0 proposal #45269
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@ntedgi @RafaelGSS
lib: promise version of streams.finished call clean up
cc2393c
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 danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@ntedgi @danielleadams
lib: promise version of streams.finished call clean up
8e21580
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
@ntedgi @danielleadams
lib: promise version of streams.finished call clean up
4ab1530
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]>
@Renegade334 Renegade334 mentioned this pull request Sep 21, 2024
doc: correct cleanup option in stream.(promises.)finished #55043
Merged
nodejs-github-bot pushed a commit that referenced this pull request Sep 25, 2024
@Renegade334
doc: correct cleanup option in stream.(promises.)finished
7f48081
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
@Renegade334 @targos
doc: correct cleanup option in stream.(promises.)finished
73b05cf
PR-URL: #55043
Refs: #44862
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MoLow MoLow MoLow left review comments
@mcollina mcollina mcollina approved these changes
@ronag ronag ronag approved these changes
@jasnell jasnell Awaiting requested review from jasnell

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@ntedgi @nodejs-github-bot @OmiCoding @mcollina @jasnell @ronag @MoLow
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.