child_process: execFile and fork arg parsing ambiguity

[jasnell]

See: #2667 (comment)

When it lands, #2667 will bring over the v0.12 changes that improved argument parsing in child_process execFile and fork. However, even with that change, argument parsing is still too ambiguous and ought to be tightened up. For instance, execFile('ls',{a:1},'test') goes through without a throw even tho a string is passed in for the callback. Internally, execFile just acts as if the callback wasn't provided at all. Likewise, execFile('ls',[], 'test', function() {}) does not throw either. Nor does execFile('ls',[],'test','test'). The only type that is actually checked is the args, so that if you pass in execFile('ls', 'test'), a TypeError will throw.

For v4.0.0, the priority is on landing #2667 to ensure parity with v0.12, but moving forward we'll want to tighten up the argument parsing here (and likely in other places as well).

@trevnorris (@nodejs/api)

[ChuckLangford]

@jasnell: Would you rather see the argument parsing in execFile be replaced? Or just made more complete?

[jasnell]

Either really. If there's going to be argument checking it needs to be consistent.

[ChuckLangford]

@jasnell: So I was running through the contribution guide and I got up to the testing section. I'm seeing odd behavior that I'm hoping you can clear up for me. I ran ./configure && make -j8 test and everything passed. I ran the same command without a code change and I started seeing errors. When I ran the same command for a third time, it ran without error.

I then switched to the unaltered and up-to-date master branch and ran the same command multiple times. Master also exhibited the same behavior.

Before I go diving into tests that are seemingly unrelated, can you tell me if this is a known issue?

Here's an example of a few of the errors I'm seeing. The errors themselves are inconsistent too; different builds result in different errors.

=== release test-net-connect-local-error ===
Path: parallel/test-net-connect-local-error
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: undefined == 12346
    at Socket.onError (/node/test/parallel/test-net-connect-local-error.js:13:10)
    at Socket.<anonymous> (/node/test/common.js:395:15)
    at emitOne (events.js:78:13)
    at Socket.emit (events.js:170:7)
    at emitErrorNT (net.js:1258:8)
    at nextTickCallbackWith2Args (node.js:470:9)
    at process._tickCallback (node.js:384:17)
Command: out/Release/node /node/test/parallel/test-net-connect-local-error.js



=== release test-cluster-net-send ===
Path: parallel/test-cluster-net-send
[23511] master
[23519] worker

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at process.<anonymous> (/node/test/parallel/test-cluster-net-send.js:29:12)
    at process.g (events.js:264:16)
    at emitOne (events.js:83:20)
    at process.emit (events.js:170:7)
Command: out/Release/node /node/test/parallel/test-cluster-net-send.js

[jasnell]

There are some tests that are known to be flaky on some operating systems.
If you're seeing inconsistent behavior, you've likely stumbled upon one.
The easiest thing to do to confirm is to open an issue describing the
behavior you're seeing and see if others have seen the same behavior.
On Dec 30, 2015 1:48 PM, "Chuck Langford" notifications@github.com wrote:

@jasnell https://github.com/jasnell: So I was running through the
contribution guide and I got up to the testing section. I'm seeing odd
behavior that I'm hoping you can clear up for me. I ran ./configure &&
make -j8 test and everything passed. I ran the same command without a
code change and I started seeing errors. When I ran the same command for a
third time, it ran without error.

I then switched to the unaltered and up-to-date master branch and ran the
same command multiple times. Master also exhibited the same behavior.

Before I go diving into tests that are seemingly unrelated, can you tell
me if this is a known issue?

Here's an example of a few of the errors I'm seeing. The errors themselves
are inconsistent too; different builds result in different errors.

=== release test-net-connect-local-error ===
Path: parallel/test-net-connect-local-error
assert.js:89
throw new assert.AssertionError({
^
AssertionError: undefined == 12346
at Socket.onError (/node/test/parallel/test-net-connect-local-error.js:13:10)
at Socket. (/node/test/common.js:395:15)
at emitOne (events.js:78:13)
at Socket.emit (events.js:170:7)
at emitErrorNT (net.js:1258:8)
at nextTickCallbackWith2Args (node.js:470:9)
at process._tickCallback (node.js:384:17)
Command: out/Release/node /node/test/parallel/test-net-connect-local-error.js

=== release test-cluster-net-send ===
Path: parallel/test-cluster-net-send
[23511] master
[23519] worker

assert.js:89
throw new assert.AssertionError({
^
AssertionError: false == true
at process. (/node/test/parallel/test-cluster-net-send.js:29:12)
at process.g (events.js:264:16)
at emitOne (events.js:83:20)
at process.emit (events.js:170:7)
Command: out/Release/node /node/test/parallel/test-cluster-net-send.js

—
Reply to this email directly or view it on GitHub
#2681 (comment).

[Trott]

Proposed fix in #7399