fs: fix createReadStream(…, {end: n}) for non-seekable fds

82bdf8fba2 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 82bdf8fba2 fixed,
and align behaviour with the native file stream mechanism
introduced in https://github.com/nodejs/node/pull/18936 as well.

Backport-PR-URL: https://github.com/nodejs/node/pull/19411
PR-URL: https://github.com/nodejs/node/pull/19329
Fixes: https://github.com/nodejs/node/issues/19240
Refs: https://github.com/nodejs/node/pull/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>

lib/fs.js

@@ -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;
@@ -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();
 
@@ -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.

test/parallel/test-fs-read-stream.js

@@ -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');
@@ -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() {
@@ -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', () => {});