Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Piping twice to same stream then unpiping loses data #12718

Closed
ovikholt opened this issue Apr 28, 2017 · 3 comments
Closed

Piping twice to same stream then unpiping loses data #12718

ovikholt opened this issue Apr 28, 2017 · 3 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@ovikholt
Copy link

ovikholt commented Apr 28, 2017

  • Version: v7.7.2
  • Platform: Darwin ter.local 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 23 18:25:34 PDT 2016; root:xnu-3248.60.10~1/RELEASE_X86_64 x86_64
  • Subsystem: stream

Piping twice to one single destination, and then unpiping once, has unexpected consequences. See the below code example.

var stream = require('stream');

var passThrough = new stream.PassThrough();
var writable = process.stdout;  // same results with fs.createWriteStream('somefile.txt')

passThrough.pipe(writable);
passThrough.pipe(writable);  // remove this line to see output
// At this point, as we expect, two writables are listening.
// (passThrough._events.data.length == 2)

passThrough.unpipe(writable);
// At this point, no writable is listening to the data event any more!
// (passThrough._events.data is undefined)

// But, passThrough does have pipeCount=1, and that pipe is our writable.
// The passThrough stream is flowing aka resumed...

passThrough.write('this does not get buffered, and thus gets lost\n');
passThrough.pipe(writable);
// Prints nothing

If the intended behavior of unpiping once is to leave one destination in place, the data listener for that destination must be retained, so that our writable will keep receiving data.

Otherwise, I assume the intended behavior of unpiping once is to remove all pipes to the
same destination, (and pausing the stream when there are no remaining pipes).

If it's invalid to pipe twice to one destination, the documentation should say so and/or the program should crash.

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Apr 28, 2017
@addaleax
Copy link
Member

If it's invalid to pipe twice to one destination, the documentation should say so and/or the program should crash.

I’m not sure, what would you expect to be the result of piping twice? Having all output of the source stream duplicated? That might lead to odd behaviour because streams don’t guarantee that the chunk boundaries stay the same, so unless you’re in object mode you might end up with non-deterministic behaviour, I think.

@benjamingr
Copy link
Member

I’m not sure, what would you expect to be the result of piping twice?

Presumably, they're expecting either to reduce the pipeCount to 0 from 2 (and not 1) and pause the pipe -or to reduce the pipeCount to 1 and still keep one listener.

Alternatively, to document that passing through to the same output twice produces non-deterministic behavior and is not supported.

I'd probably go for the latter, but I'd like to see a motivating example for piping to the same output twice from a passthrough stream.

@addaleax
Copy link
Member

PR for fixing: #12746

addaleax added a commit to addaleax/node that referenced this issue May 2, 2017
Fix the uncommon situation when a readable stream is piped twice into
the same destination stream, and then unpiped once.

Previously, the `unpipe` event handlers weren’t able to tell whether
they were corresponding to the “right” conceptual pipe that was being
removed; this fixes this by adding a counter to the `unpipe` event
handler and only removing a single piping destination at most.

Fixes: nodejs#12718
anchnk pushed a commit to anchnk/node that referenced this issue May 6, 2017
Fix the uncommon situation when a readable stream is piped twice into
the same destination stream, and then unpiped once.

Previously, the `unpipe` event handlers weren’t able to tell whether
they were corresponding to the “right” conceptual pipe that was being
removed; this fixes this by adding a counter to the `unpipe` event
handler and only removing a single piping destination at most.

Fixes: nodejs#12718
PR-URL: nodejs#12746
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 16, 2018
Fix the uncommon situation when a readable stream is piped twice into
the same destination stream, and then unpiped once.

Previously, the `unpipe` event handlers weren’t able to tell whether
they were corresponding to the “right” conceptual pipe that was being
removed; this fixes this by adding a counter to the `unpipe` event
handler and only removing a single piping destination at most.

Fixes: #12718
PR-URL: #12746
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants