errors, child_process: use more internal/errors codes #14998

maclover7 · 2017-08-24T01:33:22Z

Converts more (I believe this is all that is remaining?) of child_process to new-errors-land. Most of this diff is straight copy and replace, so I don't think it's too intimidating. There are a few TODOs left in this PR on purpose that require some discussion, so I would appreciate any feedback on those.

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)

errors, child_process

jasnell · 2017-08-24T04:35:40Z

lib/child_process.js

-      throw new TypeError('Incorrect value of args option');
+      throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
+                                 ('arguments[' + pos + ']'),
+                                 arguments[pos]);


This one feels super odd to me... will think about this a bit more.

Felt odd to me too, and still mulling over how to properly present this state to users 😞

Shouldn't this be a ERR_INVALID_ARG_TYPE though? Judging from the code below I would say it's options that has the wrong type?

jasnell · 2017-08-24T04:36:39Z

lib/child_process.js

-    throw new TypeError('Forked processes must have an IPC channel');
+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
+                               'options.stdio',
+                               options.stdio);


I think a more specific error is better for this.
e.g. new errors.Error('ERR_CHILD_PROCESS_IPC_REQUIRED') with the original error message in tact.

jasnell · 2017-08-24T04:38:18Z

lib/child_process.js

@@ -318,6 +323,8 @@ exports.execFile = function(file /*, args, options, callback*/) {
      stdoutLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length;

      if (stdoutLen > options.maxBuffer) {
+        // TODO helpful to convert to ERR_BUFFER_OUT_OF_BOUNDS?
+        // can't specify stderr/stdout with that new error.
        ex = new Error('stdout maxBuffer exceeded');


I would make this a RangeError with a specific (slightly improved) error message... e.g.

new errors.RangeError('ERR_CHILD_PROCESS_STDOUT_MAXBUFFER') // 'stdout maxBuffer length exceeded'

Would have to create ERR_CHILD_PROCESS_STDERR_MAXBUFFER in addition to ERR_CHILD_PROCESS_STDOUT_MAXBUFFER, is this ok?

Or create a single ERR_CHILD_PROCESS_STDIO_MAXBUFFER that takes a single argument specifying 'stdout' or 'stderr'

jasnell · 2017-08-24T04:39:50Z

test/parallel/test-child-process-fork-stdio-string-variant.js

+const expectedError =
+  common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError });
+
+assert.throws(() => fork(childScript, malFormedOpts), expectedError);


It's now possible to combine these into ...

common.expectsError(() => fork(...), { code: 'ERR_INVALID_OPT_VALUE', type: TypeError });

etc

jasnell

This is definitely heading in the right direction! Thank you so very much! I've left a few comments and will do another thorough review a bit later

maclover7 · 2017-08-30T16:54:06Z

@jasnell PTAL

BridgeAR · 2017-09-13T18:45:38Z

Ping @jasnell
This could use some general LGs @nodejs/collaborators

jasnell · 2017-09-15T22:48:38Z

Ping. this needs more @nodejs/tsc review.

joyeecheung · 2017-09-16T02:04:40Z

lib/child_process.js

@@ -192,7 +197,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
  }

  if (!callback && pos < arguments.length && arguments[pos] != null) {
-    throw new TypeError('Incorrect value of args option');
+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', arguments);


IIUC the error message would be The value "['file', [], '']" is invalid for option "args" if we do execFile('file', [], ''), but then those are all the arguments, not args (which is [] in this case)

thefourtheye · 2017-09-16T01:59:29Z

doc/api/errors.md

+<a id="ERR_CHILD_PROCESS_STDIO_MAXBUFFER"></a>
+### ERR_CHILD_PROCESS_STDIO_MAXBUFFER
+
+Used when the main process is trying to read data from the child process'


child process's

thefourtheye · 2017-09-16T02:04:42Z

lib/child_process.js

@@ -61,7 +62,9 @@ exports.fork = function(modulePath /*, args, options*/) {

  if (pos < arguments.length && arguments[pos] != null) {
    if (typeof arguments[pos] !== 'object') {
-      throw new TypeError('Incorrect value of args option');
+      throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
+                                 ('arguments[' + pos + ']'),


Nit: A template literal would be better here I guess.

thefourtheye · 2017-09-16T02:06:28Z

lib/child_process.js

@@ -612,14 +643,22 @@ exports.execSync = execSync;

 function validateTimeout(timeout) {
  if (timeout != null && !(Number.isInteger(timeout) && timeout >= 0)) {
-    throw new TypeError('"timeout" must be an unsigned integer');
+    // TODO should this be a RangeError?


The old error message seems okay to me.

thefourtheye · 2017-09-16T02:06:41Z

lib/child_process.js

+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
+                               'options.maxBuffer',
+                               maxBuffer);
+    //'"maxBuffer" must be a positive number');


Not needed.

thefourtheye · 2017-09-16T02:06:45Z

lib/child_process.js

+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
+                               'options.timeout',
+                               timeout);
+    //"timeout" must be an unsigned integer');


Not needed.

thefourtheye · 2017-09-16T02:09:41Z

test/parallel/test-child-process-spawn-typeerror.js


 assert.throws(function() {
-  const file = { toString() { throw new Error('foo'); } };
+  const file = { toString() { return null; } };


Is this intentional? What happens now if the error is thrown like in the old test?

thefourtheye · 2017-09-16T02:16:26Z

test/parallel/test-child-process-spawnsync-input.js

-
+common.expectsError(
+  () => spawnSync('cat', [], options),
+  { code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 1);


Just making an observation. This is the only place in this patch where expectsError is explicitly passed 1.

joyeecheung · 2017-09-16T02:06:20Z

lib/child_process.js

@@ -375,13 +382,13 @@ function _convertCustomFds(options) {

 function normalizeSpawnArguments(file, args, options) {
  if (typeof file !== 'string' || file.length === 0)
-    throw new TypeError('"file" argument must be a non-empty string');
+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'file', file);


This should be ERR_INVALID_ARG_TYPE

joyeecheung · 2017-09-16T02:07:46Z

lib/child_process.js


  if (Array.isArray(args)) {
    args = args.slice(0);
  } else if (args !== undefined &&
             (args === null || typeof args !== 'object')) {
-    throw new TypeError('Incorrect value of args option');
+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'args', args);


This should be ERR_INVALID_ARG_TYPE

joyeecheung · 2017-09-16T02:14:34Z

lib/child_process.js

@@ -612,14 +643,22 @@ exports.execSync = execSync;

 function validateTimeout(timeout) {
  if (timeout != null && !(Number.isInteger(timeout) && timeout >= 0)) {
-    throw new TypeError('"timeout" must be an unsigned integer');
+    // TODO should this be a RangeError?


Yeah...I think this should be a RangeError.

joyeecheung · 2017-09-16T02:16:12Z

lib/child_process.js

  }
 }


 function validateMaxBuffer(maxBuffer) {
  if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) {
-    throw new TypeError('"maxBuffer" must be a positive number');
+    // TODO should this be a RangeError?
+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE',


This is a RangeError as well

BridgeAR · 2017-09-19T20:52:30Z

Ping @maclover7

BridgeAR · 2017-09-24T15:35:28Z

@maclover7 do you still want to pursue this? There is overlapping PR that waits for this one to be merged.

maclover7 · 2017-09-26T00:59:46Z

@BridgeAR sorry for my delay -- working on addressing comments. make seems to be taking a little longer lately since lots of changes in master :)

BridgeAR

Overall LGTM but the the commented code should be removed.

BridgeAR · 2017-09-27T21:46:04Z

lib/internal/errors.js

+E('ERR_CHILD_PROCESS_IPC_REQUIRED',
+  'Forked processes must have an IPC channel');
+E('ERR_CHILD_PROCESS_STDIO_MAXBUFFER',
+  (name) => `${name} maxBuffer length exceeded`);


Nit - to keep it consistent with the other types it should use %s maxBuffer length exceeded. That is not blocking though.

BridgeAR · 2017-09-27T21:46:45Z

test/parallel/test-child-process-spawnsync-validation-errors.js

 }

 {
  // Validate the maxBuffer option
-  const err = /^TypeError: "maxBuffer" must be a positive number$/;
+  //const err = /^TypeError: "maxBuffer" must be a positive number$/;


This should be removed.

BridgeAR · 2017-09-27T21:47:57Z

@nodejs/tsc this needs some LGs

joyeecheung · 2017-09-28T03:14:42Z

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

BridgeAR · 2017-09-28T04:31:35Z

Rerun of the CI https://ci.nodejs.org/job/node-test-pull-request/10304/

BridgeAR

LGTM and this is ready to land besides the failing Windows test.

BridgeAR · 2017-09-28T05:35:00Z

test/parallel/test-child-process-spawnsync-validation-errors.js

@@ -4,6 +4,12 @@ const assert = require('assert');
 const spawnSync = require('child_process').spawnSync;
 const signals = process.binding('constants').os.signals;

+const invalidArgTypeError =
+  common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56);


On Windows there are only 36 entries because a couple tests are not run on Windows.

See also https://ci.nodejs.org/job/node-test-binary-windows/11411/COMPILED_BY=vs2015,RUNNER=win2008r2-vs2017,RUN_SUBSET=3/console

jasnell · 2017-10-30T01:17:04Z

@maclover7 ... should this be ready to go ?

maclover7 · 2017-11-28T20:21:07Z

rebased, one last CI before landing: https://ci.nodejs.org/job/node-test-pull-request/11783/

maclover7 · 2017-11-28T21:03:17Z

New CI after fixing linter error: https://ci.nodejs.org/job/node-test-pull-request/11784/

apapirovski · 2017-11-28T21:13:23Z

lib/internal/errors.js

@@ -272,6 +272,9 @@ E('ERR_BUFFER_TOO_LARGE',
  `Cannot create a Buffer larger than 0x${kMaxLength.toString(16)} bytes`);
 E('ERR_CANNOT_WATCH_SIGINT', 'Cannot watch for SIGINT signals');
 E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received');
+E('ERR_CHILD_PROCESS_IPC_REQUIRED',
+  'Forked processes must have an IPC channel, %s is %s');


This is used in one place, it doesn't feel like %s is %s is really that helpful. It should probably just say something like Forked processes must have an IPC channel, missing value 'ipc' in %s.

(This accepts an array so the %s would be [value, value, value].)

maclover7 · 2017-11-29T15:04:11Z

@apapirovski updated PTAL

maclover7 · 2017-11-29T22:57:09Z

Waiting for CI to wrap up: https://ci.nodejs.org/job/node-test-pull-request/11808/

maclover7 · 2017-11-29T23:06:34Z

Landing

maclover7 · 2017-11-29T23:57:31Z

Landed in b1e6c0d

PR-URL: #14998 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Aug 24, 2017

Trott added semver-major PRs that contain breaking changes and should be released in the next major version. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Aug 24, 2017

jasnell reviewed Aug 24, 2017

View reviewed changes

jasnell requested changes Aug 24, 2017

View reviewed changes

targos mentioned this pull request Aug 30, 2017

lib/child_process: convert to using internal/errors #15090

Closed

jasnell approved these changes Sep 13, 2017

View reviewed changes

joyeecheung reviewed Sep 16, 2017

View reviewed changes

thefourtheye reviewed Sep 16, 2017

View reviewed changes

joyeecheung reviewed Sep 16, 2017

View reviewed changes

maclover7 force-pushed the jm-errors-childprocess branch from 6f1c996 to 601549a Compare September 26, 2017 02:03

BridgeAR approved these changes Sep 27, 2017

View reviewed changes

BridgeAR requested a review from a team September 27, 2017 21:48

joyeecheung approved these changes Sep 28, 2017

View reviewed changes

BridgeAR requested changes Sep 28, 2017

View reviewed changes

maclover7 force-pushed the jm-errors-childprocess branch from a5ee4d8 to 17e304d Compare October 30, 2017 02:15

maclover7 added 11 commits November 28, 2017 15:19

[squash] simply & rm one-off expectedError vars

bd60e81

[squash] Create new ERR_CHILD_PROCESS_IPC_REQUIRED

4790d46

[squash] introduce new ERR_CHILD_PROCESS_STDIO_MAXBUFFER error

876321a

[squash] fix lots of comments

2d08284

[squash] fix more nits

f9a5dd6

[squash] ERR_INVALID_OPT_VALUE --> ERR_INVALID_ARG_TYPE

dba4e7c

[squash] remove comment

12a1889

[squash] change call count for windows

cc218d8

[squash] isWindows is a prop, not function; use let for error

6ecf0db

ERR_INVALID_OPT_VALUE --> ERR_INVALID_ARG_VALUE

b9b0843

better error message

c48150a

fix lint error, switch to util.format style

6084ef4

apapirovski reviewed Nov 28, 2017

View reviewed changes

slightly modify error message

8c3eba6

apapirovski approved these changes Nov 29, 2017

View reviewed changes

maclover7 self-assigned this Nov 29, 2017

maclover7 closed this Nov 29, 2017

maclover7 deleted the jm-errors-childprocess branch November 29, 2017 23:57

gibfahn mentioned this pull request Dec 4, 2017

suggestion: investigate a commit-queue solution nodejs/build#705

Closed

errors, child_process: use more internal/errors codes #14998

errors, child_process: use more internal/errors codes #14998

Conversation

maclover7 commented Aug 24, 2017

Checklist

Affected core subsystem(s)

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

maclover7 commented Aug 30, 2017

BridgeAR commented Sep 13, 2017

jasnell commented Sep 15, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 24, 2017

maclover7 commented Sep 26, 2017

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 commented Sep 27, 2017

joyeecheung commented Sep 28, 2017

BridgeAR commented Sep 28, 2017

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

jasnell commented Oct 30, 2017

maclover7 commented Nov 28, 2017

maclover7 commented Nov 28, 2017

apapirovski Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

maclover7 commented Nov 29, 2017 • edited Loading

maclover7 commented Nov 29, 2017

maclover7 commented Nov 29, 2017

maclover7 commented Nov 29, 2017

apapirovski Nov 28, 2017 •

edited

Loading

maclover7 commented Nov 29, 2017 •

edited

Loading