fs.createReadStream doesn't throw with options={start:NaN,end:NaN}

[anliting]

• Version: v9.10.1
• Platform: Linux pca 4.15.13-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Sun Mar 25 11:27:57 UTC 2018 x86_64 GNU/Linux
• Subsystem: fs

I think it would be better to throw in the following case. The following case also causes every newly created read stream to end immediately.

$ node --experimental-modules a.mjs with a.mjs:

import fs from 'fs'
function length(rs){
    return new Promise(res=>{
        let a=[]
        rs.on('data',[].push.bind(a))
        rs.on('end',e=>{
            res(Buffer.concat(a).length)
        })
    })
}
fs.writeFileSync('hello.txt','world')
;(async()=>{
    // print 5 0 0
    console.log(
        await length(fs.createReadStream('hello.txt')),
        await length(fs.createReadStream('hello.txt',{start:NaN,end:NaN})),
        await length(fs.createReadStream('hello.txt')),
    )
})()

[ryzokuken]

If I'm not wrong, this should just involve adding a simple check at https://github.com/nodejs/node/blob/master/lib/fs.js#L1976, something on the lines of:

if (Number.isNaN(options.start) || Number.isNaN(options.end)) {
  throw new Error(...) // Perhaps a RangeError would be more fitting?
}

Let me know if it sounds about right and I'll submit a PR promptly.

[anliting]

Similar checking can be found here (https://github.com/nodejs/node/blob/master/lib/fs.js#L2010-L2026), so I think it might be more appropriate to make a patch here, for the sake of maintainability.

start and end are integers (https://nodejs.org/api/fs.html#fs_fs_createreadstream_path_options), so I think it might be more appropriate to check it with Number.isInteger, avoiding there are similar cases like this issue we didn't find.

I would fix it like this: https://github.com/anliting/node-1/blob/master/lib/fs.js#L2010-L2030

[ryzokuken]

@anliting I've made the change at the exact same place.

[ghost]

isNaN still allows infinite numbers (-Infinity, Infinity).
Infinity is currently used as the fallback value for end for backwards compatibility, but I am not so sure if it's a good choice for start.

[anliting]

@arboreal84 Check out #19732 if it solves your issue.

[ryzokuken]

@arboreal84 I don't think Number.isInteger(Infinity) or Number.isInteger(-Infinity) return true though.