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

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

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

@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.

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

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

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

@nodejs/streams

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

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?

Anyone able to take a look please?

Who can help with that? I have no clue.

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.

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.

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

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

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.

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

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

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

#34035
#35122

Also relevant:

#35640

@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.

lgtm

what is the diff vs what we have on master?

what is the diff vs what we have on master?

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

@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?

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

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.)

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

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

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>

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

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

Uh oh!

Uh oh!

[v14.x backport] stream: simpler and faster Readable async iterator - #34887

[v14.x backport] stream: simpler and faster Readable async iterator #34887ronag wants to merge 6 commits intonodejs:v14.x-stagingnodejs/node:v14.x-stagingfrom nxtedition:backport-34035-to-v14.xnxtedition/node:backport-34035-to-v14.xCopy head branch name to clipboard

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

[v14.x backport] stream: simpler and faster Readable async iterator #34887
ronag wants to merge 6 commits into
nodejs:v14.x-stagingfrom
nxtedition:backport-34035-to-v14.x

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