New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: spawn ignores options in case args is undefined #24913

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@eduardbcom
Copy link
Contributor

eduardbcom commented Dec 8, 2018

spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: #24912

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Dec 8, 2018

Can you add a test for this?

@@ -403,8 +403,9 @@ function normalizeSpawnArguments(file, args, options) {

if (Array.isArray(args)) {
args = args.slice(0);
} else if (args !== undefined &&
(args === null || typeof args !== 'object')) {
} else if (args === undefined) {

This comment has been minimized.

@Trott

Trott Dec 8, 2018

Member

Any reason not to be generous and permit null here? I believe our linter permits == null which evaluates to true for both null and undefined. In fact, there are two other places in this file already that use that.

This comment has been minimized.

@eduardbcom

eduardbcom Dec 9, 2018

Contributor

Adjusted that case.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 9, 2018

Here's what the test file could look like.

'use strict';
const common = require('../common');

const assert = require('assert');
const { spawn } = require('child_process');

const tmpdir = require('../common/tmpdir');

const command = common.isWindows ? 'cd' : 'pwd';

const subprocess = spawn(command, undefined, { cwd: tmpdir.path });

subprocess.stdout.on('data', common.mustCall((data) => {
  const actual = data.toString().trim().toLowerCase();
  const expected = tmpdir.path.trim().toLowerCase();
  assert.strictEqual(actual, expected);
}));

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from 1b28329 to f52d957 Dec 9, 2018

@eduardbcom

This comment has been minimized.

Copy link
Contributor

eduardbcom commented Dec 9, 2018

Updated PR.

// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 9, 2018

Member

New tests should not include the copyright boilerplate.

This comment has been minimized.

@eduardbcom

eduardbcom Dec 9, 2018

Contributor

+

const subprocess = spawn(command, testCase, { cwd: tmpdir.path });

subprocess.stdout.on('data', common.mustCall((data) => {
resolve(data.toString().trim().toLowerCase());

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 9, 2018

Member

Use subprocess.stdout.setEncoding('utf8') so you get strings instead of buffers and accumulate the strings.

toString-ing a buffer is unsound because the output may be split over multiple buffers (i.e., you can get multiple 'data' events and multi-byte sequences may be split across buffers.)

This comment has been minimized.

@eduardbcom

eduardbcom Dec 9, 2018

Contributor

Good catch, overlooked that

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from f52d957 to 2f8a34c Dec 9, 2018

@eduardbcom

This comment has been minimized.

Copy link
Contributor

eduardbcom commented Dec 9, 2018

PR has been updated according to comments.

@Trott

This comment has been minimized.

@Trott

Trott approved these changes Dec 9, 2018

Copy link
Member

Trott left a comment

LGTM as-is or with the comment I suggested adding.

@mscdex mscdex referenced this pull request Dec 9, 2018

Open

lib: extract validateObject validator #24920

2 of 2 tasks complete

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from 2f8a34c to f57a30b Dec 10, 2018

@Trott

This comment has been minimized.

@Trott Trott added the author ready label Dec 10, 2018

@sam-github
Copy link
Member

sam-github left a comment

See comment.

@@ -0,0 +1,46 @@
'use strict';

This comment has been minimized.

@sam-github

sam-github Dec 10, 2018

Member

There is already a test for this behaviour, could you expand it instead of adding another test file? See test-child-process-spawn-typeerror.js (and yes, it's name doesn't well reflect is purpose anymore). I assume this change effects, or SHOULD effect, every child_process API that accepts an argv argument, showing that they have identical handling of the argv option, which is what the test I mentioned is set up for.

This comment has been minimized.

@Trott

Trott Dec 11, 2018

Member

@sam-github Nit or requirement to land?

This comment has been minimized.

@sam-github

sam-github Dec 12, 2018

Member

I'd rather it lands with these changes. Particularly with API consistency.

This comment has been minimized.

@eduardbcom

eduardbcom Dec 12, 2018

Contributor

That's a little bit controversial.

This file contains lots of unrelated checks, like https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-spawn-typeerror.js#L115. Why do we test execFile function within spawn test file, shouldn't it be placed in test-child-process-exec-file.js or something like that ?

And we have already separate test file test-child-process-spawn-argv0.js for testing argv0 param, so what the problem with args ?

In my opinion, *-spawn-typeerror.js should contain tests just about incorrect types, and just for spawn method.

This comment has been minimized.

@eduardbcom

eduardbcom Dec 12, 2018

Contributor

In addition, that fix effects only spawn and spawnSync methods.

This comment has been minimized.

@Trott

Trott Dec 12, 2018

Member

I also favor smaller, more-focused test files, especially for child_process tests in parallel.

This comment has been minimized.

@sam-github

sam-github Dec 12, 2018

Member

I consider that tests that are intended to ensure multiple APIs have behave identically, when they should, are best contained within a single file.

At this time, I can't tell from the tests whether all the child_process APIs do, in fact, have identical arg handling. Also, I can't tell if this PR makes them more (or less) consistent. Its possible that it fixes an inconsistency. Its possible it makes them inconsistent. I don't know. Ideally, the test structure would make that very clear. Every PR doesn't have to be ideal. I'm removing my review.

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch 3 times, most recently from 89d8237 to 982fc17 Dec 12, 2018

@@ -0,0 +1,38 @@
'use strict';

This comment has been minimized.

@eduardbcom

eduardbcom Dec 12, 2018

Contributor

Added same test for spawnSync method.

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from 982fc17 to 18d4a95 Dec 12, 2018

Not reviewing.

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 14, 2018

Looks like there are relevant test failures on Windows:

15:40:38 not ok 64 parallel/test-child-process-spawn-args
15:40:38   ---
15:40:38   duration_ms: 0.251
15:40:38   severity: fail
15:40:38   exitcode: 1
15:40:38   stack: |-
15:40:38     events.js:174
15:40:38           throw er; // Unhandled 'error' event
15:40:38           ^
15:40:38     
15:40:38     Error: spawn cd ENOENT
15:40:38         at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
15:40:38         at onErrorNT (internal/child_process.js:422:16)
15:40:38         at internalTickCallback (internal/process/next_tick.js:72:19)
15:40:38         at process._tickCallback (internal/process/next_tick.js:47:5)
15:40:38         at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
15:40:38         at executeUserCode (internal/bootstrap/node.js:317:15)
15:40:38         at startExecution (internal/bootstrap/node.js:251:3)
15:40:38         at startup (internal/bootstrap/node.js:202:3)
15:40:38         at internal/bootstrap/node.js:682:1
15:40:38     Emitted 'error' event at:
15:40:38         at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
15:40:38         at onErrorNT (internal/child_process.js:422:16)
15:40:38         [... lines matching original stack trace ...]
15:40:38         at internal/bootstrap/node.js:682:1
15:40:38   ...
15:45:14 ok 65 parallel/test-child-process-spawn-argv0
15:45:14   ---
15:45:14   duration_ms: 0.596
15:45:14   ...
15:45:15 not ok 66 parallel/test-child-process-spawnsync-args
15:45:15   ---
15:45:15   duration_ms: 0.201
15:45:15   severity: fail
15:45:15   exitcode: 1
15:45:15   stack: |-
15:45:15     assert.js:86
15:45:15       throw new AssertionError(obj);
15:45:15       ^
15:45:15     
15:45:15     AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
15:45:15     + actual - expected
15:45:15     
15:45:15     + null
15:45:15     - Buffer [Uint8Array] []
15:45:15         at testCases.map (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-spawnsync-args.js:33:10)
15:45:15         at Array.map (<anonymous>)
15:45:15         at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-spawnsync-args.js:26:27)
15:45:15         at Module._compile (internal/modules/cjs/loader.js:718:30)
15:45:15         at Object.Module._extensions..js (internal/modules/cjs/loader.js:729:10)
15:45:15         at Module.load (internal/modules/cjs/loader.js:617:32)
15:45:15         at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
15:45:15         at Function.Module._load (internal/modules/cjs/loader.js:552:3)
15:45:15         at Function.Module.runMain (internal/modules/cjs/loader.js:771:12)
15:45:15         at executeUserCode (internal/bootstrap/node.js:317:15)
15:45:15   ...

@Trott Trott removed the author ready label Dec 14, 2018

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from 18d4a95 to 5c90c83 Dec 15, 2018

@eduardbcom

This comment has been minimized.

Copy link
Contributor

eduardbcom commented Dec 15, 2018

Fixed tests for Windows based systems

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from 5c90c83 to 9db8495 Dec 15, 2018

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 16, 2018

Still seems to be failing in relevant ways on Windows:

14:32:33 not ok 64 parallel/test-child-process-spawn-args
14:32:33   ---
14:32:33   duration_ms: 0.250
14:32:33   severity: fail
14:32:33   exitcode: 1
14:32:33   stack: |-
14:32:33     events.js:174
14:32:33           throw er; // Unhandled 'error' event
14:32:33           ^
14:32:33     
14:32:33     Error: spawn cd ENOENT
14:32:33         at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
14:32:33         at onErrorNT (internal/child_process.js:422:16)
14:32:33         at internalTickCallback (internal/process/next_tick.js:72:19)
14:32:33         at process._tickCallback (internal/process/next_tick.js:47:5)
14:32:33         at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
14:32:33         at executeUserCode (internal/bootstrap/node.js:318:15)
14:32:33         at startExecution (internal/bootstrap/node.js:252:3)
14:32:33         at startup (internal/bootstrap/node.js:203:3)
14:32:33         at internal/bootstrap/node.js:693:1
14:32:33     Emitted 'error' event at:
14:32:33         at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
14:32:33         at onErrorNT (internal/child_process.js:422:16)
14:32:33         [... lines matching original stack trace ...]
14:32:33         at internal/bootstrap/node.js:693:1
14:32:33   ...

Do you have access to a Windows machine? If not, maybe someone from @nodejs/platform-windows can help.

child_process: spawn ignores options in case args is undefined
spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: #24912

@eduardbcom eduardbcom force-pushed the eduardbcom:spawn-ignores-options-when-args-undefined branch from 9db8495 to f4e8e66 Dec 16, 2018

@eduardbcom

This comment has been minimized.

Copy link
Contributor

eduardbcom commented Dec 16, 2018

@Trott no, I don't but that typo could be the case, let's check one more time
thanks

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 16, 2018

Trott added a commit to Trott/io.js that referenced this pull request Dec 16, 2018

child_process: spawn ignores options in case args is undefined
spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: nodejs#24912

PR-URL: nodejs#24913
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 16, 2018

Landed in 2077007

@Trott Trott closed this Dec 16, 2018

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 17, 2018

The tests added in this PR fail for me locally:

$ ./node test/parallel/test-child-process-spawn-args.js
events.js:174
      throw er; // Unhandled 'error' event
      ^

Error: spawn pwd ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
    at onErrorNT (internal/child_process.js:422:16)
    at internalTickCallback (internal/process/next_tick.js:72:19)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
    at executeUserCode (internal/bootstrap/node.js:318:15)
    at startExecution (internal/bootstrap/node.js:252:3)
    at startup (internal/bootstrap/node.js:203:3)
    at internal/bootstrap/node.js:693:1
Emitted 'error' event at:
    at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
    at onErrorNT (internal/child_process.js:422:16)
    [... lines matching original stack trace ...]
    at internal/bootstrap/node.js:693:1

Am I missing something?

@eduardbcom

This comment has been minimized.

Copy link
Contributor

eduardbcom commented Dec 17, 2018

@targos

python tools/test.py -J --mode=release test/parallel/test-child-process-spawn-args.js

Details: https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 17, 2018

@targos The test is missing a tmpdir.refresh() call. It doesn't (usually) affect things when run with the Python script because it assigns a tmpdir that is typically leftover from some other test, but when running from the command line, you may be less lucky.

I'll open a PR to fix.

Trott added a commit to Trott/io.js that referenced this pull request Dec 17, 2018

test: add missing tmpdir.refresh() in recently-added test
Without `tmpdir.refresh()`, the test fails in some situations. This was
missed because using `test.py` will almost always result in a leftover
tmpdir lying around that makes the `refresh()` not needed.

Refs: nodejs#24913 (comment)

@Trott Trott referenced this pull request Dec 17, 2018

Closed

test: add missing tmpdir.refresh() in recently-added test #25098

2 of 2 tasks complete
@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 17, 2018

FWIW I initially saw the tests fail with make test -j8

Trott added a commit to Trott/io.js that referenced this pull request Dec 18, 2018

test: add missing tmpdir.refresh() in recently-added test
Without `tmpdir.refresh()`, the test fails in some situations. This was
missed because using `test.py` will almost always result in a leftover
tmpdir lying around that makes the `refresh()` not needed.

Refs: nodejs#24913 (comment)

PR-URL: nodejs#25098
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 25, 2018

child_process: spawn ignores options in case args is undefined
spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: #24912

PR-URL: #24913
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins added a commit that referenced this pull request Dec 25, 2018

test: add missing tmpdir.refresh() in recently-added test
Without `tmpdir.refresh()`, the test fails in some situations. This was
missed because using `test.py` will almost always result in a leftover
tmpdir lying around that makes the `refresh()` not needed.

Refs: #24913 (comment)

PR-URL: #25098
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 25, 2018

Merged

v11.6.0 proposal #25175

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

child_process: spawn ignores options in case args is undefined
spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: nodejs#24912

PR-URL: nodejs#24913
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

test: add missing tmpdir.refresh() in recently-added test
Without `tmpdir.refresh()`, the test fails in some situations. This was
missed because using `test.py` will almost always result in a leftover
tmpdir lying around that makes the `refresh()` not needed.

Refs: nodejs#24913 (comment)

PR-URL: nodejs#25098
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment