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: fix async iterator return when destroyed earlier #31508

Closed
wants to merge 2 commits into from

Conversation

mcollina
Copy link
Member

Unfortunately, #31314 introduced a regression that made the loop not terminate:

'use strict'

const { Readable } = require('stream')

const r = new Readable({
  read () {}
})

r.destroy()

async function run () {

  for await (let chunk of r) {
  }
}

r.on('close', () => {
  console.log('close emitted')
  run().then(() => console.log('finished'))
})

I see that as more critical as an error not being forwarded. See #31314 (comment) for more details.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@mcollina mcollina changed the title Fix no return destroyed stream: fix async iterator return when destroyed earlier Jan 25, 2020
@mcollina
Copy link
Member Author

(I've discovered this because one of my modules broke because of this change)

@nodejs-github-bot
Copy link
Collaborator

A test was missing for an async iterator created after the stream
had emitted 'close'. This was regressed by nodejs#31314.

See: nodejs#31314
@mcollina mcollina force-pushed the fix-no-return-destroyed branch from 34515ff to 638ba66 Compare January 25, 2020 11:20
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

LGTM.

An alternative is to add a closeEmitted property and sort out ‘finished’. But that’s a bigger change and maybe semver-major.

@mcollina mcollina requested a review from jasnell January 25, 2020 12:46
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Jan 26, 2020

Note, this might only be needed for 13.x if #31509 lands on master.

@Trott
Copy link
Member

Trott commented Jan 27, 2020

(I've discovered this because one of my modules broke because of this change)

Perhaps we should add it to CITGM?

@Trott Trott added stream Issues and PRs related to the stream subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 27, 2020
@nodejs-github-bot

This comment has been minimized.

@mcollina
Copy link
Member Author

Perhaps we should add it to CITGM?

I don't think it adds much right now. I had it using the node core stream module because readable-stream had async iterators still experimental. As for the fix, I just switched to use readable-stream instead.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jan 29, 2020

Landed in 64161f2...24e81d7

@Trott Trott closed this Jan 29, 2020
Trott pushed a commit that referenced this pull request Jan 29, 2020
This reverts commit d15b8ea.

PR-URL: #31508
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Trott pushed a commit that referenced this pull request Jan 29, 2020
A test was missing for an async iterator created after the stream
had emitted 'close'. This was regressed by #31314.

See: #31314

PR-URL: #31508
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ronag added a commit that referenced this pull request Feb 8, 2020
Previously finished(stream, cb) would not invoke the callback
for streams that have already finished, ended or errored
before being passed to finished(stream, cb).

PR-URL: #31509
Refs: #31508
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020

Verified

This commit was signed with the committer’s verified signature.
codebytere Shelley Vohr
This reverts commit d15b8ea.

PR-URL: #31508
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020

Verified

This commit was signed with the committer’s verified signature.
codebytere Shelley Vohr
A test was missing for an async iterator created after the stream
had emitted 'close'. This was regressed by #31314.

See: #31314

PR-URL: #31508
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
targos pushed a commit that referenced this pull request Apr 18, 2020

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso
A test was missing for an async iterator created after the stream
had emitted 'close'. This was regressed by #31314.

See: #31314

PR-URL: #31508
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants