child_process: check execFile and fork args by jasnell · Pull Request #2667 · nodejs/node

jasnell · 2015-09-02T21:22:43Z

Port of joyent/node commits:

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

@rvagg @Fishrock123 @trevnorris @nodejs/tsc

See: #2515

cjihrig · 2015-09-02T21:29:04Z

Can these be broken into separate const and let variables.

Yes. I'd prefer that actually. Just literally pulled this over straight from what is in v0.12.

jasnell · 2015-09-02T22:01:39Z

Ok. @thefourtheye @cjihrig ... nits in the test fixed

trevnorris · 2015-09-02T22:26:13Z

Is it possible that both of these fail? If neither is supposed to then mind adding a final else with something like throw new Error('UNREACHABLE'); or the like?

Well, the fall through on this particular one is required for the next check. Overall, the currently behavior in v0.12 (and this port of that code) is really just a half measure. We're not actually doing a complete check. 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.

Given that this is exactly how it behaves in v0.12 currently (for better or worse), I'd be more inclined to land it as is so we can close the convergence loop first, then make additional improvements separately.

Thanks for the explanation. Sounds like a good plan. Maybe put a todo comment in the code, or we can open an issue for what you've explained and land the fix in a future master. From how you've described it, there is ambiguity with argument parsing.

Can we pull the common pos < arguments.length check outside?

not sure it's worth it to add the nested conditional. performance-wise there's no measurable gain.

so basically, I don't think it's necessary but I'm also not opposed to the change.

Right, but it will improve the readability and reduce redundant checks. It's just a suggestion I am okay without that as well.

Given that we'll need to revisit this anyway later. Let's leave it as is
for now and do any additional cleanups in the next round.
On Sep 2, 2015 10:29 PM, "thefourtheye" notifications@github.com wrote:

In lib/child_process.js
#2667 (comment):

@@ -118,18 +120,26 @@ exports.execFile = function(file /, args, options, callback/) {
env: null
};

// Parse the parameters.

// Parse the optional positional parameters.

var pos = 1;

if (pos < arguments.length && Array.isArray(arguments[pos])) {

args = arguments[pos++];

} else if (pos < arguments.length && arguments[pos] == null) {

pos++;

}

Right, but it will improve the readability and reduce redundant checks.
It's just a suggestion I am okay without that as well.

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/2667/files#r38614155.

Cool. Since we introduced new exception, this would be semver-major?

Technically. But this is already in v0.12 and is part of the convergence work. It will land in master and will be cherry picked to v4.x

Fishrock123 · 2015-09-03T01:15:13Z

@jasnell master is implicit. That label was for stuff back when master wasn't the primary branch.

Port of joyent/node commits: * nodejs/node-v0.x-archive@e17c5a7 * nodejs/node-v0.x-archive@70dafa7 Pull over test-child-process-spawn-typeerror.js from v0.12, replacing the existing test in master. The new test includes a broader set of tests on the various arg choices and throws.

Fixing multiple style nits in the test pulled over from v0.12.

jasnell · 2015-09-03T03:21:54Z

Rebased.

thefourtheye · 2015-09-03T05:40:55Z

const child

not seeing this edit as being critical to land this. can do this in the next pass.

Sure. No problem. We can actually remove the variable and directly attach the even handler.

thefourtheye · 2015-09-03T06:10:36Z

LGTM. But I would prefer waiting till we get one or more of them. Meanwhile let's have a CI run.

jasnell · 2015-09-03T06:14:18Z

Will get a CI run and get this landed in the morning. This is on the short list of must-do's for v4.0.0 so we definitely need to get this landed soon.

CI run started here: https://ci.nodejs.org/job/node-test-pull-request/240/

jasnell · 2015-09-03T06:35:14Z

CI is looking all green except for an unrelated jenkins failure on one of the windows boxes. The arm tests are still running.

thefourtheye · 2015-09-03T06:39:24Z

@jasnell That windows slave went offline during the build. Can we give it one more try. cc @nodejs/build

jasnell · 2015-09-03T06:41:24Z

Given that the CI will run when I go to land the PR, I'll wait for that. For now, it's off to bed for me.

thefourtheye · 2015-09-03T06:53:37Z

@jasnell No problem. I'll start one more run when the current one finishes. Night night :-)

cjihrig · 2015-09-03T12:50:07Z

LGTM

thefourtheye · 2015-09-03T12:52:18Z

New test run: https://ci.nodejs.org/job/node-test-pull-request/242/

jasnell · 2015-09-03T15:41:38Z

Queued up to land. There are four builds lined up before this one so it may take a while.

orangemocha · 2015-09-03T16:59:20Z

After consulting with @jasnell , I cancelled the CI run and re-queued it in https://ci.nodejs.org/job/node-accept-pull-request/123/ (pending), so that it has a better chance of succeeding if hopefully #2679 lands first.

Port of joyent/node commits: * nodejs/node-v0.x-archive@e17c5a7 * nodejs/node-v0.x-archive@70dafa7 Pull over test-child-process-spawn-typeerror.js from v0.12, replacing the existing test in master. The new test includes a broader set of tests on the various arg choices and throws. Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org> Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com> Reviewed-By: thefourtheye - Sakthipriyan Vairamani PR-URL: #2667 Fixes: #2515

jasnell · 2015-09-03T17:56:03Z

Landed (manually) in 107cbd6 (cc @rvagg @orangemocha )

thefourtheye · 2015-09-03T18:36:31Z

@jasnell you missed my email in the commit log :D

jasnell · 2015-09-03T18:44:06Z

Ugh. Sorry about that. That's why we like automated processes! (When they
work lol)
On Sep 3, 2015 11:36 AM, "thefourtheye" notifications@github.com wrote:

@jasnell https://github.com/jasnell you missed my email in the commit
log :D

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

thefourtheye · 2015-09-03T18:50:27Z

No problem :-) Glad that the patch landed at the earliest.

Fishrock123 added the child_process Issues and PRs related to the child_process subsystem. label Sep 2, 2015

cjihrig reviewed Sep 2, 2015
View reviewed changes

jasnell added land-on-4x labels Sep 2, 2015

jasnell added this to the 4.0.0 milestone Sep 2, 2015

trevnorris reviewed Sep 2, 2015
View reviewed changes

Fishrock123 removed the land-on-master label Sep 3, 2015

jasnell added 2 commits September 2, 2015 20:19

test: fix style nits in test-child-process-spawn-typeerror.js

7e5710a

Fixing multiple style nits in the test pulled over from v0.12.

jasnell force-pushed the child_process_argument_checking branch from aa0f2c0 to 7e5710a Compare September 3, 2015 03:21

rvagg mentioned this pull request Sep 3, 2015

Node.js v4 Release Timeline #2522

Closed

10 tasks

thefourtheye reviewed Sep 3, 2015
View reviewed changes

jasnell mentioned this pull request Sep 3, 2015

child_process: execFile and fork arg parsing ambiguity #2681

Closed

jasnell closed this Sep 3, 2015

This was referenced Sep 5, 2015

Release proposal: v3.3.1 #2698

Merged

[Converge] child_process argument type checking #2515

Closed

rvagg mentioned this pull request Sep 7, 2015

Release proposal: v4.0.0 #2715

Merged

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

Uh oh!

Uh oh!

Conversation

jasnell commented Sep 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

thefourtheye commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

thefourtheye commented Sep 3, 2015

Uh oh!

cjihrig commented Sep 3, 2015

Uh oh!

thefourtheye commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

orangemocha commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

thefourtheye commented Sep 3, 2015

Uh oh!

jasnell commented Sep 3, 2015

Uh oh!

thefourtheye commented Sep 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants