From e507f93c74c02ecc6e63c18d797bfeeca167155d Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Mon, 9 Apr 2018 23:02:08 +0530 Subject: [PATCH 01/26] fs: improve argument handling for ReadStream Improve handling of erratic arguments in fs.ReadStream Refs: https://github.com/nodejs/node/pull/19732 --- lib/fs.js | 66 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6c8417030b7ac7..3abe0c4f23cbc4 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2013,31 +2013,55 @@ function ReadStream(path, options) { this.bytesRead = 0; this.closed = false; - if (this.start !== undefined) { - if (typeof this.start !== 'number' || Number.isNaN(this.start)) { - throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start); - } - if (this.end === undefined) { - this.end = Infinity; - } else if (typeof this.end !== 'number' || Number.isNaN(this.end)) { - throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); - } + // Case 0: If either start or end is undefined, set them to defaults + // (0, Infinity) respectively. + this.start = this.start === undefined ? 0 : this.start; + this.end = this.end === undefined ? Infinity : this.end; - if (this.start > this.end) { - const errVal = `{start: ${this.start}, end: ${this.end}}`; - throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal); - } + // Case 1: If the type of either start or end is not number, throw + // ERR_INVALID_ARG_TYPE (TypeError). + if (typeof this.start !== 'number') { + throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start); + } + if (typeof this.end !== 'number') { + throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); + } - this.pos = this.start; + // Case 2: If either start or end is NaN, throw ERR_OUT_OF_RANGE (RangeError). + if (Number.isNaN(this.start)) { + throw new ERR_OUT_OF_RANGE('start', 'not NaN', this.start); + } + if (Number.isNaN(this.end)) { + throw new ERR_OUT_OF_RANGE('end', 'not NaN', this.end); } - // 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; - else if (Number.isNaN(this.end)) - throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); + // Case 3: If either start or end is negative, throw ERR_OUT_OF_RANGE + // (RangeError). + if (this.start < 0) { + throw new ERR_OUT_OF_RANGE('start', 'positive', this.start); + } + if (this.end < 0) { + throw new ERR_OUT_OF_RANGE('end', 'positive', this.end); + } + + // Case 4: If either start or end is fractional, round them to the nearest + // whole number. (Not integer as negatives not allowed) + if (!Number.isInteger(this.start)) { + this.start = Math.round(this.start); + } + if (!Number.isInteger(this.end)) { + this.end = Math.round(this.end); + } + + // Case 5: If start and end are both whole numbers, work perfectly. + + // Case 6: If start is greater than end, throw ERR_OUT_OF_RANGE (RangeError) + if (this.start > this.end) { + const errVal = `{start: ${this.start}, end: ${this.end}}`; + throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal); + } + + this.pos = this.start; if (typeof this.fd !== 'number') this.open(); From 30c285fc882d655373c6c6f4384ebf4744ea88a6 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Tue, 10 Apr 2018 16:05:37 +0530 Subject: [PATCH 02/26] test,fs: update tests for improvements in ReadStream Add new tests and update existing ones reflecting the changes in how ReadStream handles the `start` and `end` arguments. --- .../test-fs-read-stream-throw-type-error.js | 65 ++++++++++++++----- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js index 5cba76fa394636..03aa306fffcb7d 100644 --- a/test/parallel/test-fs-read-stream-throw-type-error.js +++ b/test/parallel/test-fs-read-stream-throw-type-error.js @@ -13,23 +13,54 @@ fs.createReadStream(example, null); fs.createReadStream(example, 'utf8'); fs.createReadStream(example, { encoding: 'utf8' }); -const createReadStreamErr = (path, opt) => { - common.expectsError( - () => { - fs.createReadStream(path, opt); - }, - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError - }); +const createReadStreamErr = (path, opt, error) => { + common.expectsError(() => { + fs.createReadStream(path, opt); + }, error); }; -createReadStreamErr(example, 123); -createReadStreamErr(example, 0); -createReadStreamErr(example, true); -createReadStreamErr(example, false); +const typeError = { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError +}; + +const rangeError = { + code: 'ERR_OUT_OF_RANGE', + type: RangeError +}; + +createReadStreamErr(example, 123, typeError); +createReadStreamErr(example, 0, typeError); +createReadStreamErr(example, true, typeError); +createReadStreamErr(example, false, typeError); + +// Case 0: Should not throw if either start or end is undefined +fs.createReadStream(example, {}); +fs.createReadStream(example, { start: 0 }); +fs.createReadStream(example, { end: Infinity }); + +// Case 1: Should throw TypeError if either start or end is not of type 'number' +createReadStreamErr(example, { start: 'invalid' }, typeError); +createReadStreamErr(example, { end: 'invalid' }, typeError); +createReadStreamErr(example, { start: 'invalid', end: 'invalid' }, typeError); + +// Case 2: Should throw RangeError if either start or end is NaN +createReadStreamErr(example, { start: NaN }, rangeError); +createReadStreamErr(example, { end: NaN }, rangeError); +createReadStreamErr(example, { start: NaN, end: NaN }, rangeError); + +// Case 3: Should throw RangeError if either start or end is negative +createReadStreamErr(example, { start: -1 }, rangeError); +createReadStreamErr(example, { end: -1 }, rangeError); +createReadStreamErr(example, { start: -1, end: -1 }, rangeError); + +// Case 4: Should not throw if either start or end is fractional +fs.createReadStream(example, { start: 0.1 }); +fs.createReadStream(example, { end: 0.1 }); +fs.createReadStream(example, { start: 0.1, end: 0.1 }); + +// Case 5: Should not throw if both start and end are whole numbers +fs.createReadStream(example, { start: 1, end: 5 }); -// createReadSteam _should_ throw on NaN -createReadStreamErr(example, { start: NaN }); -createReadStreamErr(example, { end: NaN }); -createReadStreamErr(example, { start: NaN, end: NaN }); +// Case 6: Should throw RangeError if start is greater than end +createReadStreamErr(example, { start: 5, end: 1 }, rangeError); From 915d77c2a41dbd04807475dfec25926050ce4cee Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 11 Apr 2018 02:13:02 +0530 Subject: [PATCH 03/26] Remove inconsistency --- lib/fs.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 3abe0c4f23cbc4..32d555c93e3fab 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2013,9 +2013,7 @@ function ReadStream(path, options) { this.bytesRead = 0; this.closed = false; - // Case 0: If either start or end is undefined, set them to defaults - // (0, Infinity) respectively. - this.start = this.start === undefined ? 0 : this.start; + // Case 0: If end is undefined, set it to default (Infinity) this.end = this.end === undefined ? Infinity : this.end; // Case 1: If the type of either start or end is not number, throw From 17d6d55b473cd2fc704565cdcf452c725a64dfcc Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 11 Apr 2018 02:35:29 +0530 Subject: [PATCH 04/26] Separate testing for start and end --- lib/fs.js | 96 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 57 insertions(+), 39 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 32d555c93e3fab..ae1a0a8e3de6af 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2013,53 +2013,71 @@ function ReadStream(path, options) { this.bytesRead = 0; this.closed = false; - // Case 0: If end is undefined, set it to default (Infinity) - this.end = this.end === undefined ? Infinity : this.end; + if (this.start !== undefined) { + // Case 1: If the type of start is not 'number', throw ERR_INVALID_ARG_TYPE + // (TypeError). + if (typeof this.start !== 'number') { + throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start); + } - // Case 1: If the type of either start or end is not number, throw - // ERR_INVALID_ARG_TYPE (TypeError). - if (typeof this.start !== 'number') { - throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start); - } - if (typeof this.end !== 'number') { - throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); - } + // Case 2: If start is NaN, throw ERR_OUT_OF_RANGE (RangeError). + if (Number.isNaN(this.start)) { + throw new ERR_OUT_OF_RANGE('start', 'not NaN', this.start); + } - // Case 2: If either start or end is NaN, throw ERR_OUT_OF_RANGE (RangeError). - if (Number.isNaN(this.start)) { - throw new ERR_OUT_OF_RANGE('start', 'not NaN', this.start); - } - if (Number.isNaN(this.end)) { - throw new ERR_OUT_OF_RANGE('end', 'not NaN', this.end); - } + // Case 3: If start is negative, throw ERR_OUT_OF_RANGE (RangeError). + if (this.start < 0) { + throw new ERR_OUT_OF_RANGE('start', 'positive', this.start); + } - // Case 3: If either start or end is negative, throw ERR_OUT_OF_RANGE - // (RangeError). - if (this.start < 0) { - throw new ERR_OUT_OF_RANGE('start', 'positive', this.start); - } - if (this.end < 0) { - throw new ERR_OUT_OF_RANGE('end', 'positive', this.end); - } + // Case 4: If start is fractional, round them to the nearest whole number. + // (Not integer as negatives not allowed) + if (!Number.isInteger(this.start)) { + this.start = Math.round(this.start); + } + if (!Number.isInteger(this.end)) { + this.end = Math.round(this.end); + } - // Case 4: If either start or end is fractional, round them to the nearest - // whole number. (Not integer as negatives not allowed) - if (!Number.isInteger(this.start)) { - this.start = Math.round(this.start); - } - if (!Number.isInteger(this.end)) { - this.end = Math.round(this.end); - } + // Case 5: If start is a whole number, work perfectly. - // Case 5: If start and end are both whole numbers, work perfectly. + // Case 6: If start is greater than end, throw ERR_OUT_OF_RANGE (RangeError) + if (this.start > this.end) { + const errVal = `{start: ${this.start}, end: ${this.end}}`; + throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal); + } - // Case 6: If start is greater than end, throw ERR_OUT_OF_RANGE (RangeError) - if (this.start > this.end) { - const errVal = `{start: ${this.start}, end: ${this.end}}`; - throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal); + this.pos = this.start; } - this.pos = this.start; + // Case 0: If end is undefined, set end to Infinity. + if (this.end === undefined) { + this.end = Infinity; + } else { + // Case 1: If the type of end is not 'number', throw ERR_INVALID_ARG_TYPE + // (TypeError). + if (typeof this.end !== 'number') { + throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end); + } + + // Case 2: If end is NaN, throw ERR_OUT_OF_RANGE (RangeError). + if (Number.isNaN(this.end)) { + throw new ERR_OUT_OF_RANGE('end', 'not NaN', this.end); + } + + // Case 3: If end is negative, throw ERR_OUT_OF_RANGE (RangeError). + if (this.end < 0) { + throw new ERR_OUT_OF_RANGE('end', 'positive', this.end); + } + + // Case 4: If end is fractional, round them to the nearest whole number. + // (Not integer as negatives not allowed) + if (!Number.isInteger(this.end)) { + this.end = Math.round(this.end); + } + + // Case 5: If end is a whole number, work perfectly. + } if (typeof this.fd !== 'number') this.open(); From 36574f46bc48d8a93e6f2697dcdb703eeb7de19e Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 11 Apr 2018 02:50:15 +0530 Subject: [PATCH 05/26] Remove invalid check --- lib/fs.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index ae1a0a8e3de6af..90d316763eaddf 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2035,9 +2035,6 @@ function ReadStream(path, options) { if (!Number.isInteger(this.start)) { this.start = Math.round(this.start); } - if (!Number.isInteger(this.end)) { - this.end = Math.round(this.end); - } // Case 5: If start is a whole number, work perfectly. From e4415d5075cae3490cdb2f1a182e011a379b1b56 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 11 Apr 2018 03:09:00 +0530 Subject: [PATCH 06/26] Address comments regarding handling undefined --- lib/fs.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 90d316763eaddf..4e897d48b59de8 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2013,6 +2013,9 @@ function ReadStream(path, options) { this.bytesRead = 0; this.closed = false; + // Case 0: If end is undefined, set end to Infinity. + this.end = this.end === undefined ? Infinity : this.end; + if (this.start !== undefined) { // Case 1: If the type of start is not 'number', throw ERR_INVALID_ARG_TYPE // (TypeError). @@ -2047,10 +2050,7 @@ function ReadStream(path, options) { this.pos = this.start; } - // Case 0: If end is undefined, set end to Infinity. - if (this.end === undefined) { - this.end = Infinity; - } else { + if (this.end !== undefined) { // Case 1: If the type of end is not 'number', throw ERR_INVALID_ARG_TYPE // (TypeError). if (typeof this.end !== 'number') { From b9466160e870d215dd1b14392e636bf4bdc1d7cc Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 11 Apr 2018 09:43:44 +0530 Subject: [PATCH 07/26] Refactor conditions --- lib/fs.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 4e897d48b59de8..263ed564f5cacc 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2013,9 +2013,6 @@ function ReadStream(path, options) { this.bytesRead = 0; this.closed = false; - // Case 0: If end is undefined, set end to Infinity. - this.end = this.end === undefined ? Infinity : this.end; - if (this.start !== undefined) { // Case 1: If the type of start is not 'number', throw ERR_INVALID_ARG_TYPE // (TypeError). @@ -2041,16 +2038,13 @@ function ReadStream(path, options) { // Case 5: If start is a whole number, work perfectly. - // Case 6: If start is greater than end, throw ERR_OUT_OF_RANGE (RangeError) - if (this.start > this.end) { - const errVal = `{start: ${this.start}, end: ${this.end}}`; - throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal); - } - this.pos = this.start; } - if (this.end !== undefined) { + // Case 0: If end is undefined, set end to Infinity. + if (this.end === undefined) { + this.end = Infinity; + } else { // Case 1: If the type of end is not 'number', throw ERR_INVALID_ARG_TYPE // (TypeError). if (typeof this.end !== 'number') { @@ -2074,6 +2068,12 @@ function ReadStream(path, options) { } // Case 5: If end is a whole number, work perfectly. + + // Case 6: If start is greater than end, throw ERR_OUT_OF_RANGE (RangeError) + if (this.start !== undefined && this.start > this.end) { + const errVal = `{start: ${this.start}, end: ${this.end}}`; + throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal); + } } if (typeof this.fd !== 'number') From d00b46a033472767fd34f67793aea766718cce76 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Sat, 14 Apr 2018 01:19:31 +0530 Subject: [PATCH 08/26] fs,doc: add documentation regarding changes to createReadStream Reflect the changes made to the fs.createReadStream function to the documentation at doc/api/fs.md Ref: https://github.com/nodejs/node/pull/19898 Ref: https://github.com/nodejs/node/pull/19898#issue-180377160 --- doc/api/fs.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/doc/api/fs.md b/doc/api/fs.md index 6fc7075d182451..6c714724a5e0ee 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1325,6 +1325,19 @@ start counting at 0. If `fd` is specified and `start` is omitted or `undefined`, `fs.createReadStream()` reads sequentially from the current file position. The `encoding` can be any one of those accepted by [`Buffer`][]. +The following rules apply to the `start` and `end` values: + +- If `end` is omitted or set to undefined, then it will be coerced to `Infinity`. +- The type of both `start` and `end` needs to be `number`. If not, then the +function would throw a `TypeError` with the code `ERR_INVALID_ARG_TYPE`. +- Both `start` and `end` should be valid values (i.e. they should neither be +negative nor `NaN`, and `start` should not be greater than `end`). If they are, +the function would throw a `RangeError` with the code `ERR_OUT_OF_RANGE`. +- Both `start` and `end` should be integer values. If they are not, then they +are rounded to the closest integer using `Math.round`. +- If none of the above rules apply, then the function works without any quirks +as expected. + If `fd` is specified, `ReadStream` will ignore the `path` argument and will use the specified file descriptor. This means that no `'open'` event will be emitted. Note that `fd` should be blocking; non-blocking `fd`s should be passed From 93b0915afe8cfcbaa2a0aee507e8bbf2bd501e5f Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Sat, 14 Apr 2018 20:13:26 +0530 Subject: [PATCH 09/26] Add a changes block and move new documentation towards the end --- doc/api/fs.md | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 6c714724a5e0ee..ebdb73c494fdfb 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1291,6 +1291,11 @@ fs.copyFileSync('source.txt', 'destination.txt', COPYFILE_EXCL);