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

ronag · 2020-04-21T11:36:34Z

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#32954Fixes: nodejs#32955

ronag · 2020-04-21T11:37:59Z

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

ronag · 2020-04-21T11:38:34Z

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

mcollina

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

cc @mafintosh

mcollina · 2020-04-21T11:49:50Z

which versions should this backported to v13 and v14?

vweevers · 2020-04-21T11:50:01Z

Should we add tar-stream to citgm?

ronag · 2020-04-21T11:50:56Z

which versions should this backported to v13 and v14?

v13 and v14

nodejs-github-bot · 2020-04-21T11:51:46Z

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

mafintosh · 2020-04-21T12:52:32Z

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

mafintosh · 2020-04-21T14:21:01Z

Confirmed this fixes the tar-stream regression.

mafintosh · 2020-04-21T14:33:12Z

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

ronag · 2020-04-21T14:54:19Z

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.

mcollina

lgtm

mcollina · 2020-04-21T15:02:38Z

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

cc @nodejs/tsc

nodejs-github-bot · 2020-04-21T15:26:42Z

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/

nodejs-github-bot · 2020-04-21T16:49:44Z

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#32954Fixes: nodejs#32955 PR-URL: nodejs#32968 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mathias Buus <[email protected]> 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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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: #32954Fixes: #32955 PR-URL: #32968 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mathias Buus <[email protected]>

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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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: #32954Fixes: #32955 PR-URL: #32968 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mathias Buus <[email protected]>

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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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) PR-URL: #33103

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 4d03befCompare April 21, 2020 11:44

fixup: test
04b1e3d

ronag force-pushed the stream-pipeline-destroy2 branch from 4d03bef to 04b1e3dCompare 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

Uh oh!

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

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

Uh oh!

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

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