FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: spawn ignores options in case args is undefined by eduardbme · Pull Request #24913 · nodejs/node · GitHub

/ node Public

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

Closed
eduardbme wants to merge 1 commit into
nodejs:masterfrom
eduardbme:spawn-ignores-options-when-args-undefined
Closed

child_process: spawn ignores options in case args is undefined#24913
eduardbme wants to merge 1 commit into
nodejs:masterfrom
eduardbme:spawn-ignores-options-when-args-undefined

Conversation

Copy link
Copy Markdown
Contributor

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

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

mscdex commented Dec 8, 2018

Copy link
Copy Markdown
Contributor

Can you add a test for this?

Comment thread lib/child_process.js Outdated

Trott Dec 8, 2018
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Adjusted that case.

Trott commented Dec 9, 2018
edited
Loading

Copy link
Copy Markdown
Member

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

eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from 1b28329 to f52d957 Compare December 9, 2018 10:49

Copy link
Copy Markdown
Contributor Author

Updated PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

New tests should not include the copyright boilerplate.

eduardbme Dec 9, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good catch, overlooked that

eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from f52d957 to 2f8a34c Compare December 9, 2018 16:20

Copy link
Copy Markdown
Contributor Author

PR has been updated according to comments.

Trott commented Dec 9, 2018

Copy link
Copy Markdown
Member

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from 2f8a34c to f57a30b Compare December 10, 2018 09:05

Trott commented Dec 10, 2018

Copy link
Copy Markdown
Member

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 10, 2018
sam-github previously approved these changes Dec 10, 2018

sam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

See comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@sam-github Nit or requirement to land?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

eduardbme Dec 12, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch 3 times, most recently from 89d8237 to 982fc17 Compare December 12, 2018 08:34

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added same test for spawnSync method.

eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from 982fc17 to 18d4a95 Compare December 12, 2018 09:14
sam-github dismissed their stale review December 12, 2018 17:07

Not reviewing.

Trott commented Dec 12, 2018

Copy link
Copy Markdown
Member

Trott commented Dec 14, 2018

Copy link
Copy Markdown
Member

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 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 14, 2018
eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from 18d4a95 to 5c90c83 Compare December 15, 2018 15:04

eduardbme commented Dec 15, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

Fixed tests for Windows based systems

eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from 5c90c83 to 9db8495 Compare December 15, 2018 15:07

Trott commented Dec 15, 2018

Copy link
Copy Markdown
Member

Trott commented Dec 16, 2018

Copy link
Copy Markdown
Member

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.

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

Fixes: nodejs#24912
eduardbme force-pushed the spawn-ignores-options-when-args-undefined branch from 9db8495 to f4e8e66 Compare December 16, 2018 09:12

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/19578/

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 16, 2018
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 commented Dec 16, 2018

Copy link
Copy Markdown
Member

Landed in 2077007

Trott closed this Dec 16, 2018

targos commented Dec 17, 2018

Copy link
Copy Markdown
Member

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?

eduardbme commented Dec 17, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 commented Dec 17, 2018
edited
Loading

Copy link
Copy Markdown
Member

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

targos commented Dec 17, 2018

Copy link
Copy Markdown
Member

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
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 pushed a commit that referenced this pull request Dec 25, 2018
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 pushed a commit that referenced this pull request Dec 25, 2018
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 mentioned this pull request Dec 25, 2018
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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>
BethGriggs pushed a commit that referenced this pull request Mar 14, 2019
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>
BethGriggs pushed a commit that referenced this pull request Mar 14, 2019
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>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs mentioned this pull request May 1, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChildProcess spawn ignores options param

9 participants


Back | FazBrowse Home | New Git URL