child_process: normalize fork function arguments #20882

shobhitchittora · 2018-05-22T08:18:26Z

Closes: #20749

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

arg typecheck test run - python ./tools/test.py parallel/test-child-process-spawn-typeerror.js

Affected subsystem(s)

child_process / fork

gireeshpunathil · 2018-05-22T08:27:27Z

potential user land breakage, so I suggest a CITGM run.

gireeshpunathil · 2018-05-22T09:33:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15014/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1423/

mscdex · 2018-05-22T12:57:21Z

lib/child_process.js

+              args === null ||
+              typeof args !== 'object')
+  ) {
+    // throw new ERR_INVALID_ARG_TYPE('args', 'object', args);


This should be removed

mscdex · 2018-05-22T13:02:18Z

lib/child_process.js

+  } else if (typeof options !== 'object') {
+    throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
+  } else {
+    options = {};


This is overwriting options if it is already an object or is null. My guess is this block is only meant for the latter case?

If so, perhaps this block should be removed completely and || options === null added to the first conditional that sets options = {}?

shobhitchittora · 2018-05-24T12:46:42Z

@gireeshpunathil @mscdex I made the required change and tests are now passing on my local. Would like a CI and CIGTM re-run for this.

gireeshpunathil · 2018-05-24T13:04:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15076/
let us see how CI comes out, and then we will attack CIGTM.

shobhitchittora · 2018-05-24T14:13:18Z

@gireeshpunathil some of the CIs are failing. This is the console outpot of node-test-commit-linux -

09:30:36 make[1]: *** [Makefile:448: test-ci] Error 1
09:30:36 make: *** [Makefile:473: run-ci] Error 2
09:30:37 Build step 'Execute shell' marked build as failure

Can you help me with this ? 😇Is this cause of the PR changes or something with the CI itself ?

gireeshpunathil · 2018-05-24T14:35:22Z

actually, neither. Some of the failures (async-hooks/test-zlib.zlib-binding.deflate in Linux and parallel/test-child-process-fork-net on Windows) were failing off late for which there are open issues.

The error message you saw is a kind of summary in the bottom of the CI. To see the actual failure, you can go to the top of the page, and expand the full log button, and search for not ok

You may please go through all the failures and see if you are able to associate them with the PR. You can search in the repo with the base name of the failing file and see if they appear under issues, to know whether they are outstanding open issues.

Hope this helps!

BridgeAR

I am not really sure why this moved into a separate function when it is only used in a single function. I personally would continue to keep it in there. That way we have no overhead.

There is also need to add a test for what this actually resolves.

shobhitchittora · 2018-05-24T18:00:07Z

@BridgeAR I guess separating it out make it more readable. There's a lot going in there and having reasonable moving parts makes it easier to reason about. Yeah but that's just me, do tell if this is okay. I can go ahead and add test for the function. Or else I can inline the function.

Cheers! 😇

BridgeAR · 2018-05-24T18:09:22Z

I prefer to keep it in line in this case.

BridgeAR

Please add test cases for what this should solve.

BridgeAR · 2018-05-25T11:19:09Z

lib/child_process.js

-      throw new ERR_INVALID_ARG_VALUE(`arguments[${pos}]`, arguments[pos]);
-    }
+  var args = arguments[1];
+  var options = arguments[2];


Accessing arguments without checking the length can lead to OOB accesses. That is not that bad anymore but that requires quite some work for the compiler and it still slows things down. Therefore this way of doing it is not really good.

would this be better -

var pos = 1; var args = pos < arguments.length ? arguments[pos++] : []; var options = pos < arguments.length ? arguments[pos++] : {};

Likely. Just try to make sure each code path is not hit more than absolutely necessary.

BridgeAR · 2018-05-25T11:21:34Z

lib/child_process.js

+  var options = arguments[2];
+
+  if (Array.isArray(args)) {
+    args = args.slice(0);


Is the slice needed? Please add a test case for it and remove it if there is no need for it.

I'm not sure. But for consistency I took inspiration from current implementation at -

node/lib/child_process.js

Line 403 in 468ab45

args = args.slice(0);

That is not a strong argument. Please just verify this by adding a test as suggested.

BridgeAR · 2018-05-25T11:26:29Z

lib/child_process.js

+  } else if (options !== undefined &&
+    (args === undefined ||
+      args === null ||
+      typeof args !== 'object')


If args is not an object and it is not undefined or null, it should actually throw an error. Just ignoring the argument does not help anyone.

Adding a

throw new ERR_INVALID_ARG_VALUE('arg', 'object', options);

for the code you mentioned breaks this line in test -

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

Line 180 in 468ab45

fork(empty, u, o);

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

Line 182 in 468ab45

fork(empty, n, n);

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

Line 183 in 468ab45

fork(empty, n, o);

Should we update this test and add assert.thorws for these cases.

No, the test cases would continue to work but the code has to be rewritten further.

maclover7 · 2018-08-11T20:27:54Z

ping @shobhitchittora

Closes: nodejs#20749

refs: nodejs#20749

shobhitchittora · 2018-08-20T15:57:49Z

Closing and opening a new one. Trying a simpler solution.

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label May 22, 2018

shobhitchittora changed the title ~~child_process: normalize fork function arguments ( handling fork('task', undefined, {opts} )~~ child_process: normalize fork function arguments May 22, 2018

mscdex reviewed May 22, 2018

View reviewed changes

shobhitchittora force-pushed the fork-args-undefined-fix branch 2 times, most recently from fea8122 to a48fd21 Compare May 24, 2018 12:40

BridgeAR reviewed May 24, 2018

View reviewed changes

BridgeAR reviewed May 25, 2018

View reviewed changes

gireeshpunathil mentioned this pull request May 29, 2018

child_process: fork does not accept null or undefined args array #10819

Closed

maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018

shobhitchittora added 5 commits August 20, 2018 19:09

child_process: adds normalization for fork function arguments

a0d6b15

Closes: nodejs#20749

child_process: handles options === null

7a31575

refs: nodejs#20749

child_process: fixing indentation + refactored options check for null.

e1199c2

refs: nodejs#20749

child_process: removes unrelated change ( due to lint auto fix )

37d6019

refs: nodejs#20749

child_process: inlines arg normalization in the fork function itself.

529480e

refs: nodejs#20749

shobhitchittora force-pushed the fork-args-undefined-fix branch from 8778216 to 529480e Compare August 20, 2018 13:40

shobhitchittora closed this Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: normalize fork function arguments #20882

child_process: normalize fork function arguments #20882

shobhitchittora commented May 22, 2018 •

edited

gireeshpunathil commented May 22, 2018

gireeshpunathil commented May 22, 2018

mscdex May 22, 2018

mscdex May 22, 2018

shobhitchittora commented May 24, 2018

gireeshpunathil commented May 24, 2018

shobhitchittora commented May 24, 2018

gireeshpunathil commented May 24, 2018

BridgeAR left a comment

shobhitchittora commented May 24, 2018 •

edited

BridgeAR commented May 24, 2018

BridgeAR left a comment

BridgeAR May 25, 2018 •

edited

shobhitchittora May 25, 2018 •

edited

BridgeAR May 28, 2018

BridgeAR May 25, 2018

shobhitchittora May 25, 2018

BridgeAR May 28, 2018

BridgeAR May 25, 2018

shobhitchittora May 25, 2018 •

edited

BridgeAR May 28, 2018

maclover7 commented Aug 11, 2018

shobhitchittora commented Aug 20, 2018

child_process: normalize fork function arguments #20882

child_process: normalize fork function arguments #20882

Conversation

shobhitchittora commented May 22, 2018 • edited

Affected subsystem(s)

gireeshpunathil commented May 22, 2018

gireeshpunathil commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shobhitchittora commented May 24, 2018

gireeshpunathil commented May 24, 2018

shobhitchittora commented May 24, 2018

gireeshpunathil commented May 24, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

shobhitchittora commented May 24, 2018 • edited

BridgeAR commented May 24, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR May 25, 2018 • edited

Choose a reason for hiding this comment

shobhitchittora May 25, 2018 • edited

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

shobhitchittora May 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maclover7 commented Aug 11, 2018

shobhitchittora commented Aug 20, 2018

shobhitchittora commented May 22, 2018 •

edited

shobhitchittora commented May 24, 2018 •

edited

BridgeAR May 25, 2018 •

edited

shobhitchittora May 25, 2018 •

edited

shobhitchittora May 25, 2018 •

edited