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 readable stream to multiple writeable streams results in crash #5820

Closed
kr3l opened this issue Mar 21, 2016 · 7 comments
Closed

Piping readable stream to multiple writeable streams results in crash #5820

kr3l opened this issue Mar 21, 2016 · 7 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.

Comments

@kr3l
Copy link

kr3l commented Mar 21, 2016

If I execute the following code snippet in node >= 4.2.0, it results in the read stream reading the data very fast but not being able to write them out to wr2 equally fast. The memory usage of the process increases very fast until the process exits with an error.

var fs = require('fs');

//M: and L: are local disks with fast read and write
// the .mov file is several gigabytes in size
var rd = fs.createReadStream('M:/A007C003_141024_R7C6.mov');
var wr = fs.createWriteStream('L:/A007C003_141024_R7C6.mov');

//N: is a NAS drive, slow
var wr2 = fs.createWriteStream('N:/A007C003_141024_R7C6.mov');

//read from fast disk, write to both fast and slow disk
rd.pipe(wr);
rd.pipe(wr2);

The script ends with this error:

.\node-4.2.0 test.js
buffer.js:98
    const ui8 = new Uint8Array(size);
                ^

RangeError: Invalid array buffer length
    at new ArrayBuffer (native)
    at new Uint8Array (native)
    at allocate (buffer.js:98:17)
    at new Buffer (buffer.js:49:12)
    at Function.Buffer.concat (buffer.js:234:16)
    at fromList (_stream_readable.js:840:20)
    at ReadStream.Readable.read (_stream_readable.js:339:11)
    at flow (_stream_readable.js:743:26)
    at WriteStream.<anonymous> (_stream_readable.js:601:7)
    at emitNone (events.js:67:13)
    at WriteStream.emit (events.js:166:7)

Earlier versions of node (4.1.2 and below) do not show the same behaviour.

  • Version: v4.2.0
  • Platform: Win10 64bit
  • Subsystem:
@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Mar 21, 2016
@Fishrock123
Copy link
Contributor

v4.2.0 has this commit: #2325 -- #2325

Sounds like it might be related?

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Mar 21, 2016
@calvinmetcalf
Copy link
Contributor

though the error in question isn't actually an error related to running out of memory which is the error you'd expect if it was actually due to improper back pressure handling between 2 streams.

@kr3l
Copy link
Author

kr3l commented Mar 31, 2016

From the error and the observed memory behaviour, maybe the 'size' variable is becoming too big and overflowing before another memory-related error can occur?

@addaleax
Copy link
Member

There’s a maximum size of 2^31-1 bytes for a Buffer, so that would definitely make sense, even without an actual overflow occurring.

@addaleax
Copy link
Member

addaleax commented Apr 2, 2016

I’ve looked a bit into this and it seems @Fishrock123 is right, #2325 effectively – and probably inadvertently – disabled the readableState.awaitDrain functionality that’s supposed to keep the destination streams in sync (if I read the code correctly).

@Fishrock123
Copy link
Contributor

cc @mscdex here too

@addaleax
Copy link
Member

addaleax commented Apr 2, 2016

PR: #6023

addaleax added a commit to addaleax/node that referenced this issue Apr 3, 2016
In 6899094 (nodejs#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: nodejs#5820
MylesBorins pushed a commit that referenced this issue Apr 19, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 30, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 6, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 18, 2016
In 6899094 (#2325),
the conditions for increasing `readableState.awaitDrain` when
writing to a piping destination returns false were changed so
that they could not actually be met, effectively leaving
`readableState.awaitDrain` with a constant value of 0.

This patch changes the conditions to testing whether the
stream for which `.write()` returned false is still a piping
destination, which was likely the intention of the original
patch.

Fixes: #5820
Fixes: #5257
PR-URL: #6023
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants