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: fixes async iterator destroyed error propagation #31314

Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 11, 2020

There was an edge case where if _destroy calls the error callback
later than one tick the iterator would complete early and not
propgate the error.

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

@ronag
Copy link
Member Author

ronag commented Jan 11, 2020

ping @nodejs/streams bug?

@ronag ronag force-pushed the fix-async-iterator-delayed-error branch 3 times, most recently from 8613847 to 4eaa164 Compare January 11, 2020 12:44
There was an edge case where if _destroy calls the error callback
later than one tick the iterator would complete early and not
propgate the error.
@ronag ronag force-pushed the fix-async-iterator-delayed-error branch from 4eaa164 to a09e59e Compare January 11, 2020 12:54
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

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added 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. labels Jan 11, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Jan 14, 2020
There was an edge case where if _destroy calls the error callback
later than one tick the iterator would complete early and not
propgate the error.

PR-URL: #31314
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Jan 14, 2020

Landed in d15b8ea

@Trott Trott closed this Jan 14, 2020
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
There was an edge case where if _destroy calls the error callback
later than one tick the iterator would complete early and not
propgate the error.

PR-URL: #31314
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jan 16, 2020
@mcollina
Copy link
Member

Please do not backport this PR. It's causing a regression on a very simple usage of the async iterator:

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'll issue a revert and add a test for it.


This is not easy to fix, as we do not have a 'closeEmitted' or 'destroyCompleted' status, so we cannot know if the destroy callback finished. Note that .destroyed is set syncrhonously. The problem is that we call finished() waiting for the callback to fire - and it never will.
Fixing this is likely semver-major, hence the coming revert.

mcollina added a commit to mcollina/node that referenced this pull request Jan 25, 2020
A test was missing for an async iterator created after the stream
had emitted 'close'. This was regressed by nodejs#31314.

See: nodejs#31314
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ronag added a commit to nxtedition/node that referenced this pull request Feb 8, 2020
There was an edge case where if _destroy calls the error callback
later than one tick the iterator would complete early and not
propgate the error.

PR-URL: nodejs#31314
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ronag added a commit that referenced this pull request Feb 14, 2020
There was an edge case where if _destroy calls the error callback
later than one tick the iterator would complete early and not
propgate the error.

PR-URL: #31314
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>

PR-URL: #31700
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Feb 17, 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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Apr 18, 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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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.

7 participants