[v14.x backport] stream: simpler and faster Readable async iterator by ronag · Pull Request #34887 · nodejs/node

ronag · 2020-08-23T10:01:39Z

A backport of semver-major labelled PR. Was discussed with TSC who gave a go ahead to open a PR and try to gain approval from collaborators.

PR: #34035
Refs: #34680

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

Writable stream could emit 'finish' after 'close'.

PR-URL: #32933
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com

ronag · 2020-08-23T10:08:09Z

This has a dependency on #34103 which I had to include.

ronag · 2020-08-23T10:09:10Z

@MylesBorins There are two backports in this PR. Would I need to open separate PRs? The first commit doesn't make much sense to backport without the second one.

nodejs-github-bot · 2020-08-23T10:21:31Z

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

ronag · 2020-08-23T11:00:10Z

Weird test failure? AssertionError [ERR_ASSERTION]: benchmark file not running exactly one configuration in test:?

mcollina

lgtm - I think this is the right thing to do long term.

ronag · 2020-09-01T10:54:11Z

@nodejs/streams

nodejs-github-bot · 2020-09-01T15:14:42Z

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

richardlau · 2020-09-01T15:45:31Z

Weird test failure? AssertionError [ERR_ASSERTION]: benchmark file not running exactly one configuration in test:?

We're still seeing this in the CI on multiple platforms. Anyone able to take a look please?

ronag · 2020-09-03T09:33:07Z

Anyone able to take a look please?

Who can help with that? I have no clue.

mhdawson · 2020-09-03T15:10:45Z

Discussed in TSC meeting today, no objections raised by TSC members that were there. Myles, sees Matteo chiming that it is a good thing so comfortable that it land given that stream subsystem team members are signing off once ci is green. Removing the agenda tag.

lundibundi · 2020-09-03T15:43:46Z

Who can help with that? I have no clue.

~~@ronag I think you have to force a single configuration for your benchmark test in the test/benchmark/test-benchmark-streams.js~~
~~(see example in test/benchmark/test-benchmark-policy.js)~~

This is not the case since #31755.

nodejs-github-bot · 2020-10-14T10:32:39Z

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

Fixes some compatibility issues where it is expected that for await stops reading when the stream is destroyed. Refs: nodejs#34887

ronag · 2020-10-14T11:13:18Z

This needs some more collaborator review to ensure we are confident with landing it in 14.x.

BethGriggs · 2020-10-14T11:18:36Z

Just FYI in case it shows up in this PR, it looks like test-asan is failing on v14.x, i'm trying to figure that out in #35641.

ronag · 2020-10-14T15:17:05Z

@BethGriggs Green CI. Just needs another approval or two. Thanks for the ping!

mcollina · 2020-10-14T16:47:16Z

What are the commits that are backported here? Have we got a references to those two PRs?

ronag · 2020-10-14T19:33:14Z

Yea I seem to have ruined the commit msg. The backported PR's are:

#34035
#35122

Also relevant:

#35640

MylesBorins · 2020-10-14T20:00:51Z

@ronag it seems like multipel prs + commits have all merged into one here. I'm assuming that it will be a lot of work to split them up... so in lieu of that I think we could treat this as an entirely new change that avoids needing to backport the other change.

That does mean we will need at least 2 signoff before landing this.

mcollina

lgtm

mcollina · 2020-10-14T20:35:19Z

what is the diff vs what we have on master?

ronag · 2020-10-14T20:54:12Z

what is the diff vs what we have on master?

Once #35640 is merged, I believe they are exactly the same.

MylesBorins · 2020-10-14T21:11:02Z

@mcollina @ronag . Should we rush getting this into the next 14? it will be the last release before LTS, planned to go out tomorrow

#35648

I'll be honest, considering these changes caused issues in the past I'm a bit nervous about including them just before flipping the LTS bit, but perhaps I am being too conservative. What do y'all think?

mcollina · 2020-10-14T21:13:12Z

I think we would like to have this in v14 to simplify backporting.

BethGriggs · 2020-10-15T10:11:36Z

I'll be honest, considering these changes caused issues in the past I'm a bit nervous about including them just before flipping the LTS bit, but perhaps I am being too conservative. What do y'all think?

FWIW, i'm somewhat leaning the opposite way, and feel that (for the same reasons) this change should land before v14.x is promoted to LTS. (Assuming that CITGM is happy.)

MylesBorins · 2020-10-15T14:37:25Z

@ronag would you be able to rebase so we can make sure the asan test isn't broken by this PR

MylesBorins · 2020-10-15T16:50:51Z

@nodejs/streams we still need 1 more sign off on this

MylesBorins · 2020-10-15T19:41:11Z

I'm 99% sure the test failure here was broken on 14.x before this was opened and the PR just needs a rebase. I'm going to land this as soon as #35640 lands

Fixes some compatibility issues where it is expected that for await stops reading when the stream is destroyed. Refs: #34887 PR-URL: #35640 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

includes: * stream: simpler and faster Readable async iterator * stream: don't destroy on async iterator success * stream: async iterator stop read if destroyed PR-URL: #34887 Refs: #34035 Refs: #35122 Refs: #35640 Refs: #34680 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2020-10-15T19:55:23Z

Landed in 573410f

Hopefully I captures things accurately in the commit message

ronag added the stream Issues and PRs related to the stream subsystem. label Aug 23, 2020

ronag requested review from benjamingr, lpinca and mcollina August 23, 2020 10:01

nodejs-github-bot added build Issues and PRs related to build files or the CI. v14.x labels Aug 23, 2020

ronag force-pushed the backport-34035-to-v14.x branch from 91ba1df to 8fecd44 Compare August 23, 2020 10:02

ronag changed the title ~~stream: don't emit finish after close~~ [v14.x backport] stream: don't emit finish after close Aug 23, 2020

This comment has been minimized.

Sign in to view

ronag changed the title ~~[v14.x backport] stream: don't emit finish after close~~ [v14.x backport] stream: simpler and faster Readable async iterator Aug 23, 2020

ronag force-pushed the backport-34035-to-v14.x branch from 8fecd44 to 6c942b0 Compare August 23, 2020 10:07

lpinca approved these changes Aug 23, 2020

View reviewed changes

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 23, 2020

mcollina approved these changes Aug 23, 2020

View reviewed changes

This was referenced Aug 24, 2020

Node.js Technical Steering Committee (TSC) Meeting 2020-08-27 nodejs/TSC#911

Closed

Node.js Technical Steering Committee (TSC) Meeting 2020-09-03 nodejs/TSC#915

Closed

richardlau force-pushed the v14.x-staging branch from 09369a0 to fb2111e Compare September 3, 2020 11:07

mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 3, 2020

ronag requested a review from mcollina October 14, 2020 10:19

ronag commented Oct 14, 2020

View reviewed changes

Comment thread lib/_stream_readable.js

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020

ronag requested a review from lpinca October 14, 2020 10:49

ronag added a commit to nxtedition/node that referenced this pull request Oct 14, 2020

stream: async iterator stop read if destroyed

2c9432b

Fixes some compatibility issues where it is expected that for await stops reading when the stream is destroyed. Refs: nodejs#34887

ronag mentioned this pull request Oct 14, 2020

stream: async iterator stop read if destroyed #35640

Closed

4 tasks

mcollina approved these changes Oct 14, 2020

View reviewed changes

lpinca approved these changes Oct 15, 2020

View reviewed changes

MylesBorins closed this Oct 15, 2020

christhegrand mentioned this pull request Mar 23, 2021

Return function of subscription's async iterator never resolves on some versions of Node 14 graphql/graphql-js#2983

Closed

targos mentioned this pull request May 16, 2021

stream: don't emit finish after close #32933

Closed

4 tasks

Uh oh!

Conversation

ronag commented Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

This comment has been minimized.

ronag commented Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Aug 23, 2020

Uh oh!

nodejs-github-bot commented Aug 23, 2020

Uh oh!

ronag commented Aug 23, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Sep 1, 2020

Uh oh!

nodejs-github-bot commented Sep 1, 2020

Uh oh!

richardlau commented Sep 1, 2020

Uh oh!

ronag commented Sep 3, 2020

Uh oh!

mhdawson commented Sep 3, 2020

Uh oh!

lundibundi commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 14, 2020

Uh oh!

ronag commented Oct 14, 2020

Uh oh!

BethGriggs commented Oct 14, 2020

Uh oh!

ronag commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Oct 14, 2020

Uh oh!

ronag commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Oct 14, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 14, 2020

Uh oh!

ronag commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Oct 14, 2020

Uh oh!

BethGriggs commented Oct 15, 2020

Uh oh!

MylesBorins commented Oct 15, 2020

Uh oh!

MylesBorins commented Oct 15, 2020

Uh oh!

MylesBorins commented Oct 15, 2020

Uh oh!

MylesBorins commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ronag commented Aug 23, 2020 •

edited

Loading

ronag commented Aug 23, 2020 •

edited

Loading

lundibundi commented Sep 3, 2020 •

edited

Loading

ronag commented Oct 14, 2020 •

edited

Loading

ronag commented Oct 14, 2020 •

edited

Loading

ronag commented Oct 14, 2020 •

edited

Loading

MylesBorins commented Oct 14, 2020 •

edited

Loading

MylesBorins commented Oct 15, 2020 •

edited

Loading