stream: pipeline should only destroy un-finished streams. by ronag · Pull Request #32968 · nodejs/node · GitHub

This PR logically reverts #31940 which
has caused lots of unnecessary breakage in the ecosystem.

This PR also aligns better with the actual documented behavior:

stream.pipeline() will call stream.destroy(err) on all streams except:

Readable streams which have emitted 'end' or 'close'.
Writable streams which have emitted 'finish' or 'close'.

The behavior introduced in #31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.

Furthermore, it makes the code simpler and removes some hacks.

Fixes: #32954
Fixes: #32955

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

This PR logically reverts nodejs#31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in nodejs#31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: nodejs#32954 Fixes: nodejs#32955

@nodejs/streams: I'm sorry about the pain caused by #31940.

@mafintosh Could you take a look at this and see if it resolves the issues you have encountered?

lgtm, but we should test this with the offending modules.

cc @mafintosh

which versions should this backported to v13 and v14?

Should we add tar-stream to citgm?

which versions should this backported to v13 and v14?

v13 and v14

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

@ronag testing (my macbook isn't the fastest at compiling v8). also streams are tricky, and i appreciate you maintaining all this :)

Confirmed this fixes the tar-stream regression.

Don't know what the release schedule is like but can this land before 14 goes out?

Don't know what the release schedule is like but can this land before 14 goes out?

I think it's too late. v14 just got merged.

lgtm

I'm calling for a fast-track here, and get it into 14.0.1 when it gets out.

cc @nodejs/tsc

CI: https://ci.nodejs.org/job/node-test-pull-request/30887/
Defensive CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2355/

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

This PR logically reverts nodejs#31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in nodejs#31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: nodejs#32954 Fixes: nodejs#32955 PR-URL: nodejs#32968 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com> Backport-PR-URL: nodejs#32980

An unfortunate overlap between two PR that by themselves pass CI but together pass a test. nodejs#32967 changes so that pipeline does not wait for 'close'. nodejs#32968 changed so that all streams are not destroyed. Which made one test fail when expected the stream to be destroyed during pipeline callback.

An unfortunate overlap between two PR that by themselves pass CI but together pass a test. #32967 changes so that pipeline does not wait for 'close'. #32968 changed so that all streams are not destroyed. Which made one test fail when expected the stream to be destroyed during pipeline callback. PR-URL: #33030 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This PR logically reverts #31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in #31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: #32954 Fixes: #32955 PR-URL: #32968 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>

An unfortunate overlap between two PR that by themselves pass CI but together pass a test. #32967 changes so that pipeline does not wait for 'close'. #32968 changed so that all streams are not destroyed. Which made one test fail when expected the stream to be destroyed during pipeline callback. PR-URL: #33030 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable changes: - deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32971](#32971) - module: do not warn when accessing `\_\_esModule` of unfinished exports (Anna Henningsen) [#33048](#33048) - stream: - don't wait for close on legacy streams (Robert Nagy) [#33058](#33058) - pipeline should only destroy un-finished streams (Robert Nagy) [#32968](#32968) PR-URL: #33103

This PR logically reverts #31940 which has caused lots of unnecessary breakage in the ecosystem. This PR also aligns better with the actual documented behavior: `stream.pipeline()` will call `stream.destroy(err)` on all streams except: * `Readable` streams which have emitted `'end'` or `'close'`. * `Writable` streams which have emitted `'finish'` or `'close'`. The behavior introduced in #31940 was much more aggressive in terms of destroying streams. This was good for avoiding potential resources leaks however breaks some common assumputions in legacy streams. Furthermore, it makes the code simpler and removes some hacks. Fixes: #32954 Fixes: #32955 PR-URL: #32968 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>

An unfortunate overlap between two PR that by themselves pass CI but together pass a test. #32967 changes so that pipeline does not wait for 'close'. #32968 changed so that all streams are not destroyed. Which made one test fail when expected the stream to be destroyed during pipeline callback. PR-URL: #33030 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable changes: - deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32971](#32971) - doc: add juanarbol as collaborator (Juan José Arboleda) [#32906](#32906) - http: doc deprecate abort and improve docs (Robert Nagy) [#32807](#32807) - module: do not warn when accessing `__esModule` of unfinished exports (Anna Henningsen) [#33048](#33048) - n-api: detect deadlocks in thread-safe function (Gabriel Schulhof) [#32860](#32860) - src: deprecate embedder APIs with replacements (Anna Henningsen) [#32858](#32858) - stream: - don't emit end after close (Robert Nagy) [#33076](#33076) - don't wait for close on legacy streams (Robert Nagy) [#33058](#33058) - pipeline should only destroy un-finished streams (Robert Nagy) [#32968](#32968) - vm: add importModuleDynamically option to compileFunction (Gus Caplan) [#32985](#32985) PR-URL: #33103

ronag requested review from mafintosh and mcollina April 21, 2020 11:36

ronag added stream Issues and PRs related to the stream subsystem. v13.x labels Apr 21, 2020

ronag force-pushed the stream-pipeline-destroy2 branch from e1a927b to 4d03bef Compare April 21, 2020 11:44

fixup: test

04b1e3d

ronag force-pushed the stream-pipeline-destroy2 branch from 4d03bef to 04b1e3d Compare April 21, 2020 11:44

fixup: simplify

6b92c99

mcollina approved these changes Apr 21, 2020

View reviewed changes

ronag added the v14.x label Apr 21, 2020

fixup: unnecessary once

76906a5

mafintosh approved these changes Apr 21, 2020

View reviewed changes

mcollina approved these changes Apr 21, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

This was referenced Apr 21, 2020

stream: pipeline don't destroy Duplex src before 'finish' #32966

Closed

stream: cleanup pipeline destroyer #32690

Closed

ronag added the backport-open-v13.x label Apr 23, 2020

AviVahl mentioned this pull request Apr 23, 2020

Not working with Node 14 max-mapper/extract-zip#94

Closed

ronag mentioned this pull request Apr 23, 2020

stream: fix broken pipeline test #33030

Closed

4 tasks

This was referenced Apr 24, 2020

Puppeteer not working with node 14 puppeteer/puppeteer#5719

Closed

chore: log useful error for Node v14 breakage puppeteer/puppeteer#5732

Merged

BethGriggs mentioned this pull request Apr 27, 2020

v14.1.0 proposal #33103

Merged

campersau mentioned this pull request Apr 28, 2020

Migrate clicktests from nightmare to puppeteer FredrikNoren/ungit#1336

Merged

targos added the backport-blocked-v12.x label Apr 29, 2020

campersau mentioned this pull request Apr 30, 2020

Node.js 10.20.1, 12.16.3, 13.14.0, 14.1.0 appveyor/ci#3393

Closed

Pizzacus referenced this pull request in suisei-cn/hosimati.suisei.moe May 5, 2020

fix(gulp): Old-street style pipe works well!

dde81ca

tschaub mentioned this pull request May 7, 2020

Trying to fix CI openlayers/openlayers#10983

Merged

benabel mentioned this pull request May 17, 2020

Incorrect stream handling in Node v14 Lumieducation/H5P-Nodejs-library#515

Closed

keybraker mentioned this pull request Mar 22, 2021

Volume controlled opus streams do not get properly destroyed discordjs/discord.js#5429

Closed

stream: pipeline should only destroy un-finished streams. by ronag · Pull Request #32968 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: pipeline should only destroy un-finished streams. - #32968

stream: pipeline should only destroy un-finished streams.#32968ronag wants to merge 4 commits intonodejs:masternodejs/node:masterfrom nxtedition:stream-pipeline-destroy2nxtedition/node:stream-pipeline-destroy2Copy head branch name to clipboard

Conversation

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

Uh oh!

Checklist

Uh oh!

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

Uh oh!

Uh oh!

ronag commented Apr 21, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Apr 21, 2020

Uh oh!

vweevers commented Apr 21, 2020

Uh oh!

ronag commented Apr 21, 2020

Uh oh!

nodejs-github-bot commented Apr 21, 2020

Uh oh!

mafintosh commented Apr 21, 2020

Uh oh!

mafintosh commented Apr 21, 2020

Uh oh!

mafintosh commented Apr 21, 2020

Uh oh!

ronag commented Apr 21, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Apr 21, 2020

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Apr 21, 2020 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 21, 2020 • edited by ronag 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!

6 participants

stream: pipeline should only destroy un-finished streams.#32968
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-pipeline-destroy2

ronag commented Apr 21, 2020 •
edited
Loading

ronag commented Apr 21, 2020 •
edited
Loading

nodejs-github-bot commented Apr 21, 2020 •
edited by ronag
Loading

nodejs-github-bot commented Apr 21, 2020 •
edited by ronag
Loading