stream: ensure pipeline always destroys streams by ronag · Pull Request #31940 · nodejs/node · GitHub

There was an edge case where an incorrect assumption was made
in regards to whether eos/finished means that the stream is
actually destroyed or not.

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

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not.

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

This is blocking backport of #31223 to node V13 which does not have autoDestroy enabled by default, thus some tests fail.

lgtm

If this is a backport it should target the relative branch (v13-staging in this case). Moreover I would expect to see a rather different change compared to what is on master.

If this is a backport it should target the relative branch (v13-staging in this case). Moreover I would expect to see a rather different change compared to what is on master.

This is not a backport. It fixes a problem on master for streams that do not autoDestroy. However, it should be backported as well and is required for backporting #31223 to v13. Since in master we changed default autoDestroy to true which is not (and will not be) included in v13.

lgtm

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

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not. PR-URL: #31940 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Landed in b2be348

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not. Backport-PR-URL: nodejs#31975 PR-URL: nodejs#31940 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not. Backport-PR-URL: #31975 PR-URL: #31940 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@ronag It seems this diff has introduced a major regression in Got:
sindresorhus/got#1107 (comment)

Note that I don't know if it's Got that relied on a buggy behavior or if it's an actual regression, but I figured I should ref the issues together 🙂

If the last stream in a pipeline is still usable/readable don't destroy it to allow further composition. Fixes: #32105 PR-URL: #32110 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Should this land on v12.x-staging? If so, please open a backport PR.

Should not land on 12.x. Has indirect consequences that can cause breakage.

Does this work with things like a TCP stream where destroy actually destroys the underlying fd was finish still can mean its waiting for FIN ack?

Does this work with things like a TCP stream where destroy actually destroys the underlying fd was finish still can mean its waiting for FIN ack?

Sorry, I didn't get that?

simple example pipeline(socket, socket). This calls destroy automatically after eos now yes? Ie basically forces autoDestroy if I’m understanding that correctly.

For streams that have postfinish state this can be problematic. For example TCP (this may have changed since I looked at it last), the stream emits finish when all pending data has been sent but end() has a side effect. Its sends a FIN packet also. Now you auto destroy it meaning FIN wont get resend if that packet is lost.

My utp-native (tcp like stream over udp) module will def break because of this due to o/

For streams that have postfinish state this can be problematic. For example TCP (this may have changed since I looked at it last), the stream emits finish when all pending data has been sent but end() has a side effect. Its sends a FIN packet also. Now you auto destroy it meaning FIN wont get resend if that packet is lost.

I'm not familiar with this so I can't really answer. Maybe @addaleax or @jasnell?

If more work needs to be done, e.g. send a FIN packet, I think that should be handled by _destroy alternatively 'finish' should be delayed until the FIN stuff is complete (I think this might be what currently happens?).

Though, in v14 pipeline won't call destroy as it assumes that "standard" streams (such as net.Socket since #31806) will handle it themselves (#32158).

follow up: wont this break all duplex streams?

net.createServer(function (socket) {
  pipeline(socket, a)
  pipeline(b, socket)
})

Wont this destroy the socket when either of those pipelines are done?

I can see tcp uses autoDestroy so it that part is prob fine, re my first q

Wont this destroy the socket when either of those pipelines are done?

There has been follow up PRs that ensure that if:

The first stream is writable, it won't be destroyed
The last stream is readable, it won't be destroyed

The current implementation is a little bit messy, but if you look at this PR which cleans it up a bit it might be clear.

@mafintosh Anything still left unanswered?

@ronag i think the other pr solves my above issue. Didn’t realise this was older.

Have you tested this with http servers? Calling destroy on a req destroys the full socket I think

Have you tested this with http servers? Calling destroy on a req destroys the full socket I think

Yes, I noticed

Heads up, this breaks tar-stream also due to the same reasons as HTTP. Just verified on latest 13.

Test case

echo hello > hello.txt
echo world > world.txt
tar c hello.txt world.txt > test.tar

const tar = require('tar-stream')
const fs = require('fs')
const path = require('path')
const pipeline = require('stream').pipeline

fs.createReadStream('test.tar')
  .pipe(tar.extract())
  .on('entry', function (header, stream, done) {
    console.log(header.name) // in 13 this will only unpack one file due to
                                                 // pipeline destroying the entire stream
    pipeline(stream, fs.createWriteStream(path.join('/tmp', header.name)), done)
  })

Let me put that into a proper issue

Thanks for the navigation help @ronag :)

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

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>

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

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>

ronag added the stream Issues and PRs related to the stream subsystem. label Feb 24, 2020

ronag requested review from lpinca, mafintosh and mcollina February 24, 2020 22:41

stream: ensure pipeline always destroys streams

5a55b1d

There was an edge case where an incorrect assumption was made in regardos whether eos/finished means that the stream is actually destroyed or not.

ronag force-pushed the stream-pipeline-always-destroy branch from 23c5c02 to 5a55b1d Compare February 24, 2020 22:42

mcollina approved these changes Feb 25, 2020

View reviewed changes

mcollina requested changes Feb 25, 2020

View reviewed changes

mcollina approved these changes Feb 25, 2020

View reviewed changes

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 25, 2020

BridgeAR approved these changes Feb 25, 2020

View reviewed changes

lpinca approved these changes Feb 25, 2020

View reviewed changes

ronag closed this Feb 26, 2020

codebytere mentioned this pull request Mar 1, 2020

v13.10.0 proposal #32027

Merged

szmarczak mentioned this pull request Mar 5, 2020

Pipeline aborts all HTTP requests #32105

Closed

targos added the backport-requested-v12.x label Apr 20, 2020

ronag added dont-land-on-v12.x and removed backport-requested-v12.x labels Apr 20, 2020

This was referenced Apr 20, 2020

tar-stream regression in 13 with pipeline #32954

Closed

Duplex stream pipeline regression in 13 #32955

Closed

ronag mentioned this pull request Apr 21, 2020

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

Closed

4 tasks

ronag mentioned this pull request Apr 21, 2020

[v13.x backport] stream: pipeline should only destroy un-finished streams. #32980

Closed

4 tasks

stream: ensure pipeline always destroys streams by ronag · Pull Request #31940 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: ensure pipeline always destroys streams - #31940

stream: ensure pipeline always destroys streams#31940ronag wants to merge 1 commit intonodejs:masternodejs/node:masterfrom nxtedition:stream-pipeline-always-destroynxtedition/node:stream-pipeline-always-destroyCopy head branch name to clipboard

Conversation

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

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Feb 24, 2020

Uh oh!

ronag commented Feb 24, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 25, 2020

Uh oh!

ronag commented Feb 26, 2020

Uh oh!

arcanis commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Apr 20, 2020

Uh oh!

ronag commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

ronag commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

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

Uh oh!

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

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

Uh oh!

Uh oh!

ronag commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

mafintosh commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stream: ensure pipeline always destroys streams#31940
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-pipeline-always-destroy

ronag commented Feb 24, 2020 •
edited
Loading

ronag commented Feb 25, 2020 •
edited
Loading

arcanis commented Mar 5, 2020 •
edited
Loading

ronag commented Apr 20, 2020 •
edited
Loading

ronag commented Apr 20, 2020 •
edited
Loading

ronag commented Apr 20, 2020 •
edited
Loading

mafintosh commented Apr 20, 2020 •
edited
Loading