Skip to content

Commit

Permalink
fs: fix createReadStream(…, {end: n}) for non-seekable fds
Browse files Browse the repository at this point in the history
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19411
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
  • Loading branch information
addaleax authored and MylesBorins committed Mar 30, 2018
1 parent 502781c commit f81a69a
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 7 deletions.
11 changes: 9 additions & 2 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1919,8 +1919,7 @@ function ReadStream(path, options) {
this.flags = options.flags === undefined ? 'r' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

this.start = typeof this.fd !== 'number' && options.start === undefined ?
0 : options.start;
this.start = options.start;
this.end = options.end;
this.autoClose = options.autoClose === undefined ? true : options.autoClose;
this.pos = undefined;
Expand All @@ -1943,6 +1942,12 @@ function ReadStream(path, options) {
this.pos = this.start;
}

// Backwards compatibility: Make sure `end` is a number regardless of `start`.
// TODO(addaleax): Make the above typecheck not depend on `start` instead.
// (That is a semver-major change).
if (typeof this.end !== 'number')
this.end = Infinity;

if (typeof this.fd !== 'number')
this.open();

Expand Down Expand Up @@ -1996,6 +2001,8 @@ ReadStream.prototype._read = function(n) {

if (this.pos !== undefined)
toRead = Math.min(this.end - this.pos + 1, toRead);
else
toRead = Math.min(this.end - this.bytesRead + 1, toRead);

// already read everything we were supposed to read!
// treat as EOF.
Expand Down
39 changes: 34 additions & 5 deletions test/parallel/test-fs-read-stream.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';
const common = require('../common');

const child_process = require('child_process');
const assert = require('assert');

const fixtures = require('../common/fixtures');
Expand Down Expand Up @@ -146,11 +148,6 @@ stream.on('end', function() {
}));
}

// pause and then resume immediately.
const pauseRes = fs.createReadStream(rangeFile);
pauseRes.pause();
pauseRes.resume();

let file7 = fs.createReadStream(rangeFile, {autoClose: false });
file7.on('data', () => {});
file7.on('end', function() {
Expand All @@ -173,6 +170,38 @@ function file7Next() {
});
}

if (!common.isWindows) {
// Verify that end works when start is not specified, and we do not try to
// use positioned reads. This makes sure that this keeps working for
// non-seekable file descriptors.
common.refreshTmpDir();
const filename = `${common.tmpDir}/foo.pipe`;
const mkfifoResult = child_process.spawnSync('mkfifo', [filename]);
if (!mkfifoResult.error) {
child_process.exec(`echo "xyz foobar" > '${filename}'`);
const stream = new fs.createReadStream(filename, { end: 1 });
stream.data = '';

stream.on('data', function(chunk) {
stream.data += chunk;
});

stream.on('end', common.mustCall(function() {
assert.strictEqual('xy', stream.data);
fs.unlinkSync(filename);
}));
} else {
common.printSkipMessage('mkfifo not available');
}
}

{
// pause and then resume immediately.
const pauseRes = fs.createReadStream(rangeFile);
pauseRes.pause();
pauseRes.resume();
}

// Just to make sure autoClose won't close the stream because of error.
const file8 = fs.createReadStream(null, {fd: 13337, autoClose: false });
file8.on('data', () => {});
Expand Down

0 comments on commit f81a69a

Please sign in to comment.