zlib: improve zlib errors #18675

joyeecheung · 2018-02-09T11:48:12Z

zlib: improve zlib errors

Use assert to check mode in the Zlib constructor since it should
only be passed by us.
Introduce checkRangesOrGetDefault() and checkFiniteNumber()
to simplify type and range checking for numeric arguments
Instead of ERR_INVALID_OPT_VALUE, throw ERR_OUT_OF_RANGE and
ERR_INVALID_ARG_TYPE with descriptions of the expected ranges
or types to make the errors more user-friendly.
Add message tests for the changed errors

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

zlib

joyeecheung · 2018-02-09T11:49:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13055/

addaleax

Nice work!

addaleax · 2018-02-09T12:31:19Z

test/parallel/test-zlib-deflate-constructors.js

@@ -12,83 +12,201 @@ assert.ok(new zlib.Deflate() instanceof zlib.Deflate);
 assert.ok(zlib.DeflateRaw() instanceof zlib.DeflateRaw);
 assert.ok(new zlib.DeflateRaw() instanceof zlib.DeflateRaw);

-// Throws if `opts.chunkSize` is invalid
+// Throws if `optionss.chunkSize` is invalid


typo: options

BridgeAR

LGTM. Just two nits that are not blocking.

BridgeAR · 2018-02-12T14:36:58Z

lib/zlib.js

-       !Number.isFinite(strategy))) {
-    throw new errors.TypeError('ERR_INVALID_ARG_VALUE', 'strategy', strategy);
+  if (validateFiniteNumber(level, 'level')) {
+    if (level < Z_MIN_LEVEL || level > Z_MAX_LEVEL) {


The statements could be combined. So it would only say:

if (validateFiniteNumber(level, 'level') && (level < ... || level > ...)) { // ... }

The same for the one below.

@BridgeAR That does not work because a valid value would be overwritten to the default in the else block.

@BridgeAR Ah..right, this is not one of the blocks that sets default values. Nevermind.

BridgeAR · 2018-02-12T14:39:39Z

lib/zlib.js

+      if (chunkSize < Z_MIN_CHUNK) {
+        throw new errors.RangeError('ERR_OUT_OF_RANGE', 'options.chunkSize',
+                                    `>= ${Z_MIN_CHUNK}`, chunkSize);
+      }
    } else {
      chunkSize = Z_DEFAULT_CHUNK;
    }


Non blocking: A very personal point about readability: I feel like it would be better to write it as:

if (!validateFiniteNumber(...) { chunkSize = Z_... } else if (chunkSize < Z_MIN_CHUNK) { throw new ... }

joyeecheung · 2018-02-14T05:24:12Z

@BridgeAR Updated, PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/13164/

BridgeAR

LGTM, just needs a rebase and I left two suggestions.

BridgeAR · 2018-02-16T17:49:01Z

lib/zlib.js

+    throw err;
+  }
+
+  if (!Number.isFinite(number)) {


Using Number.isFinite(number) will implicitly also test for NaN and for the type number. Therefore those two checks could be moved inside of this block to make the average case faster.

Not sure I am following...NaN returns false here (no errors) while non-numbers throws a different error, how could we move those checks into this block?

Oh, sorry, you can only move the typeof number !== 'number' in here. I overlooked that NaN should be accepted and gets a default value (do we really want that? I suggest to throw an error in that case as well).

The reason why the typecheck can be moved in here is:

Number.isFinite(nonNumber) === false. So the average case does not have to test for that. Only in case the isFinite test fails, it should be checked if it is the wrong type.

I also doubt if it's necessary to use default values for NaNs, but several tests fail if I change that so I would like to investigate this behavior later.

I get what you were suggesting now. That makes sense to me, I'll change that later, thanks

BridgeAR · 2018-02-16T17:57:57Z

lib/zlib.js

+    } else if (flush < Z_NO_FLUSH || flush > Z_BLOCK) {
+      throw new errors.RangeError('ERR_OUT_OF_RANGE', 'options.flush',
+                                  `>= ${Z_NO_FLUSH} and <= ${Z_BLOCK}`,
+                                  flush);


Suggestion: all cases that check for both bounds (that are all but one if I checked correct) could also be moved in a separate functions.

That way it would look like e.g.:

if (!validateFiniteNumber(flush, 'options.flush')) { arg = foo; } else { validateRange(flush, Z_NO_FLUSH, Z_BLOCK, 'options.flush') }

Or you actually combine the check and move the range check into the validateFiniteNumber:

if (!validateNumberRange(flush, Z_NO_FLUSH, Z_BLOCK, 'options.flush')) flush = Z_NO_FLUSH;

- Use assert to check mode in the Zlib constructor since it should only be passed by us. - Introduce checkRangesOrGetDefault() and checkFiniteNumber() to simplify type and range checking for numeric arguments - Instead of `ERR_INVALID_OPT_VALUE`, throw `ERR_OUT_OF_RANGE` and `ERR_INVALID_ARG_TYPE` with descriptions of the expected ranges or types to make the errors more user-friendly. - Add message tests for the changed errors

joyeecheung · 2018-02-18T13:39:40Z

Rebased and updated. Introduced checkRangesOrGetDefault() that returns the number if it's valid, or the default if it's undefined or NaN, and throws appropriate errors otherwise. Put the range checks inside the Number.isFinite block for the common case.

CI: https://ci.nodejs.org/job/node-test-pull-request/13250/

@BridgeAR PTAL, thanks!

BridgeAR

Still LGTM but I left a few suggestions. They are not blocking though.

BridgeAR · 2018-02-18T15:23:04Z

lib/zlib.js

+// 4. Throws ERR_OUT_OF_RANGE for infinite numbers or numbers > upper or < lower
+function checkRangesOrGetDefault(number, name, lower, upper, def) {
+  if (checkFiniteNumber(number, name, lower, upper)) {
+    checkRanges(number, name, lower, upper);


checkRanges is only used here, so I guess it would be best to inline that.

BridgeAR · 2018-02-18T16:35:02Z

lib/zlib.js

-    }
+    flush = checkRangesOrGetDefault(
+      opts.flush, 'options.flush',
+      Z_NO_FLUSH, Z_BLOCK, Z_NO_FLUSH);


Hm, I just had a closer look and the values get initialized with their default value at the top of the function. So we can actually skip setting the variable again.

It is only necessary to set the variable in case a finite number got passed in. So I personally think it would still be best to have:

if (checkRange(opts.flush, 'options.flush', Z_NO_FLUSH, Z_BLOCK)) flush = opts.flush;

But setting the value again is of course also not much overhead.

BridgeAR · 2018-02-18T16:37:36Z

lib/zlib.js

+    // undefined or NaN
+    if (number === undefined || Number.isNaN(number)) {
+      return false;
+    }


I would check for undefined first because it is the cheapest check and it is the most likely one (I guess the common use case is to rely on defaults).

So:

if (number === undefined) return false; if (Number.isFinite(number) return true; if (Number.isNaN(number) return false; ...

BridgeAR · 2018-02-18T16:41:36Z

lib/zlib.js

+    // Infinite numbers
+    const err = new errors.RangeError('ERR_OUT_OF_RANGE', name,
+                                      'a finite number', number);
+    Error.captureStackTrace(err, checkFiniteNumber);


Right now this does not hide the checkRangesOrGetDefault frame anymore. This is problematic right now because the function is called from different functions and we can not just put in checkRangesOrGetDefault. I would probably just inline the checkFiniteNumber into checkRangesOrGetDefault and just have a bit of code copied for the single entry that does not fit into the pattern.

And it would be possible to combine this by doing:

let err; if (typeif number !== 'number') { err = ... } else { err = ... } Error.captureStackTrace(err, fn); throw err;

joyeecheung · 2018-02-19T18:34:16Z

Updated again, I don't feel too strongly about hiding all the frames or resetting the value so I left it there. @BridgeAR PTAL, thanks!

CI: https://ci.nodejs.org/job/node-test-pull-request/13280/

joyeecheung · 2018-02-22T13:46:08Z

Going to land this tomorrow...@BridgeAR

joyeecheung · 2018-02-24T13:01:44Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/13357/

joyeecheung · 2018-02-24T18:57:02Z

Landed in da886d9, thanks!

- Use assert to check mode in the Zlib constructor since it should only be passed by us. - Introduce checkRangesOrGetDefault() and checkFiniteNumber() to simplify type and range checking for numeric arguments - Instead of `ERR_INVALID_OPT_VALUE`, throw `ERR_OUT_OF_RANGE` and `ERR_INVALID_ARG_TYPE` with descriptions of the expected ranges or types to make the errors more user-friendly. - Add message tests for the changed errors PR-URL: #18675 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

- Use assert to check mode in the Zlib constructor since it should only be passed by us. - Introduce checkRangesOrGetDefault() and checkFiniteNumber() to simplify type and range checking for numeric arguments - Instead of `ERR_INVALID_OPT_VALUE`, throw `ERR_OUT_OF_RANGE` and `ERR_INVALID_ARG_TYPE` with descriptions of the expected ranges or types to make the errors more user-friendly. - Add message tests for the changed errors PR-URL: nodejs#18675 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Feb 9, 2018

joyeecheung added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Feb 9, 2018

addaleax approved these changes Feb 9, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

joyeecheung added semver-major PRs that contain breaking changes and should be released in the next major version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 10, 2018

daynin approved these changes Feb 10, 2018

View reviewed changes

lpinca approved these changes Feb 11, 2018

View reviewed changes

BridgeAR approved these changes Feb 12, 2018

View reviewed changes

joyeecheung force-pushed the improve-zlib-error branch from 1e05358 to 7cbfa65 Compare February 14, 2018 05:23

BridgeAR approved these changes Feb 16, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2018

joyeecheung force-pushed the improve-zlib-error branch from 7cbfa65 to 22c7c8d Compare February 18, 2018 13:39

BridgeAR approved these changes Feb 18, 2018

View reviewed changes

squash! inline checkRanges, faster return for common cases

5bd9dba

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 22, 2018

BridgeAR approved these changes Feb 22, 2018

View reviewed changes

tniessen approved these changes Feb 23, 2018

View reviewed changes

joyeecheung closed this Feb 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: improve zlib errors #18675

zlib: improve zlib errors #18675

joyeecheung commented Feb 9, 2018 •

edited

Loading

joyeecheung commented Feb 9, 2018

addaleax left a comment

addaleax Feb 9, 2018

BridgeAR left a comment

BridgeAR Feb 12, 2018

joyeecheung Feb 14, 2018 •

edited

Loading

joyeecheung Feb 14, 2018

BridgeAR Feb 12, 2018

joyeecheung commented Feb 14, 2018

BridgeAR left a comment

BridgeAR Feb 16, 2018

joyeecheung Feb 16, 2018

BridgeAR Feb 16, 2018 •

edited

Loading

joyeecheung Feb 16, 2018

BridgeAR Feb 16, 2018

joyeecheung commented Feb 18, 2018

BridgeAR left a comment

BridgeAR Feb 18, 2018

BridgeAR Feb 18, 2018

BridgeAR Feb 18, 2018

BridgeAR Feb 18, 2018

joyeecheung commented Feb 19, 2018

joyeecheung commented Feb 22, 2018

joyeecheung commented Feb 24, 2018

joyeecheung commented Feb 24, 2018

zlib: improve zlib errors #18675

zlib: improve zlib errors #18675

Conversation

joyeecheung commented Feb 9, 2018 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung commented Feb 9, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Feb 14, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Feb 18, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Feb 19, 2018

joyeecheung commented Feb 22, 2018

joyeecheung commented Feb 24, 2018

joyeecheung commented Feb 24, 2018

joyeecheung commented Feb 9, 2018 •

edited

Loading

joyeecheung Feb 14, 2018 •

edited

Loading

BridgeAR Feb 16, 2018 •

edited

Loading