child_process: normalize fork function arguments

[shobhitchittora]

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]

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

[gireeshpunathil]

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]

This should be removed

[mscdex]

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]

@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]

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]

@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]

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]

@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]

I prefer to keep it in line in this case.

[BridgeAR]

Please add test cases for what this should solve.

[BridgeAR]

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.

[shobhitchittora]

would this be better -

var pos = 1;

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

[BridgeAR]

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

[BridgeAR]

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

[shobhitchittora]

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);

[BridgeAR]

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

[BridgeAR]

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.

[shobhitchittora]

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.

[BridgeAR]

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

[maclover7]

ping @shobhitchittora

[shobhitchittora]

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