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

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. PR-URL: #19329Fixes: #19240 Refs: #18121 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Chen Gang <[email protected]>

Author: addaleax

lib/fs.js

@@ -2011,8 +2011,7 @@ function ReadStream(path, options){
 this.flags=options.flags===undefined ? 'r' : options.flags;
 this.mode=options.mode===undefined ? 0o666 : options.mode;
 
-this.start=typeofthis.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;
@@ -2046,6 +2045,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(typeofthis.end!=='number')
+this.end=Infinity;
+
 if(typeofthis.fd!=='number')
 this.open();
 
@@ -2100,6 +2105,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

@@ -21,7 +21,9 @@
 
 'use strict';
 constcommon=require('../common');
+consttmpdir=require('../common/tmpdir');
 
+constchild_process=require('child_process');
 constassert=require('assert');
 constfs=require('fs');
 constfixtures=require('../common/fixtures');
@@ -178,6 +180,31 @@ common.expectsError(
 }));
 }
 
+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.
+tmpdir.refresh();
+constfilename=`${tmpdir.path}/foo.pipe`;
+constmkfifoResult=child_process.spawnSync('mkfifo',[filename]);
+if(!mkfifoResult.error){
+child_process.exec(`echo "xyz foobar" > '${filename}'`);
+conststream=newfs.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.
 constpauseRes=fs.createReadStream(rangeFile);