Skip to content
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

doc: child_process: clarify subprocess.pid can be undefined when ENOENT #37014

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

dr-js
Copy link
Contributor

@dr-js dr-js commented Jan 21, 2021

From the code nodejs@8 and up should behave the same: https://github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290-L316

And a short test snippet:

const { spawn } = require('child_process')

const subProcess = spawn('non-exist-command')
subProcess.on('error', (error) => console.warn('mute Unhandled "error" event:', error))
console.log('- pid:', subProcess.pid)
process.nextTick(() => console.log('- after error emit'))
console.log('== end of test ==')

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jan 21, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jan 21, 2021

Do you want to update the test to make sure there's no regression in the future?

const enoentPath = 'foo123';
const spawnargs = ['bar'];
assert.strictEqual(fs.existsSync(enoentPath), false);
const enoentChild = spawn(enoentPath, spawnargs);
// Verify that stdio is setup if the error is not EMFILE or ENFILE.
assert.notStrictEqual(enoentChild.stdin, undefined);
assert.notStrictEqual(enoentChild.stdout, undefined);
assert.notStrictEqual(enoentChild.stderr, undefined);
assert(Array.isArray(enoentChild.stdio));
assert.strictEqual(enoentChild.stdio[0], enoentChild.stdin);
assert.strictEqual(enoentChild.stdio[1], enoentChild.stdout);
assert.strictEqual(enoentChild.stdio[2], enoentChild.stderr);

@dr-js
Copy link
Contributor Author

dr-js commented Jan 22, 2021

Sure, I'll try update the test.

@dr-js
Copy link
Contributor Author

dr-js commented Jan 22, 2021

@aduh95 Upon adding the test, I found the pid value is a bit more complex than I thought:

  • an async spawn with invalid cwd will get pid undefined (expected)
  • an async spawn with invalid commad and shell: false will get pid undefined (expected)
  • an async spawn with invalid commad and shell: true will get pid of the shell (expected)
  • a sync spawn seems always return pid (strange)

Should I add more detailed behavior description to the doc and update test for async and sync spawn,
or keep it undocumented behavior as it's not so straightforward for now?

The test code for this:

[test.js]
[
  () => testSpawn('with invalid command', [ 'non-exist-command' ]),
  () => testSpawn('with invalid command and `shell: true`', [ 'non-exist-command', { shell: true } ]),
  () => testSpawn('with invalid cwd', [ 'node', { cwd: './non/exist/cwd/path/' } ]),
  () => testSpawn('invalid cwd and `shell: true`', [ 'node', { cwd: './non/exist/cwd/path/', shell: true } ]),

  () => testSpawnSync('with invalid command', [ 'non-exist-command' ]),
  () => testSpawnSync('with invalid command and `shell: true`', [ 'non-exist-command', { shell: true } ]),
  () => testSpawnSync('with invalid cwd', [ 'node', { cwd: './non/exist/cwd/path/' } ]),
  () => testSpawnSync('invalid cwd and `shell: true`', [ 'node', { cwd: './non/exist/cwd/path/', shell: true } ])
].forEach((testFunc, index) => setTimeout(testFunc, index * 200))

const formatError = (error) => error && error.message
// const formatError = (error) => error && error.stack // for more detail

const testSpawn = (title = '', spawnArgs = '') => {
  console.log(`\n== test spawn ${title} ==`)
  const { spawn } = require('child_process')
  const subProcess = spawn(...spawnArgs)
  subProcess.on('error', (error) => console.warn('- "error" event:', formatError(error))) // mute Unhandled "error" event or the test will exit
  console.log('- pid:', subProcess.pid, 'exitCode:', subProcess.exitCode)
  process.nextTick(() => console.log('- nextTick pid:', subProcess.pid, 'exitCode:', subProcess.exitCode))
  setTimeout(() => console.log('- after 100ms pid:', subProcess.pid, 'exitCode:', subProcess.exitCode), 100)
}

const testSpawnSync = (title = '', spawnArgs = '') => {
  console.log(`\n== test spawnSync ${title} ==`)
  const { spawnSync } = require('child_process')
  const outcome = spawnSync(...spawnArgs) // NOTE: not a `subProcess`
  console.log('- pid:', outcome.pid, 'status', outcome.status)
  console.log('- error', formatError(outcome.error))
}

And the output:

[v15.6.0 linux x64]
v15.6.0 linux x64

== test spawn with invalid command ==
- pid: undefined exitCode: null
- "error" event: spawn non-exist-command ENOENT
- nextTick pid: undefined exitCode: -2
- after 100ms pid: undefined exitCode: -2

== test spawn with invalid command and `shell: true` ==
- pid: 30493 exitCode: null
- nextTick pid: 30493 exitCode: null
- after 100ms pid: 30493 exitCode: 127

== test spawn with invalid cwd ==
- pid: undefined exitCode: null
- "error" event: spawn node ENOENT
- nextTick pid: undefined exitCode: -2
- after 100ms pid: undefined exitCode: -2

== test spawn invalid cwd and `shell: true` ==
- pid: undefined exitCode: null
- "error" event: spawn /bin/sh ENOENT
- nextTick pid: undefined exitCode: -2
- after 100ms pid: undefined exitCode: -2

== test spawnSync with invalid command ==
- pid: 30496 status null
- error spawnSync non-exist-command ENOENT

== test spawnSync with invalid command and `shell: true` ==
- pid: 30497 status 127
- error undefined

== test spawnSync with invalid cwd ==
- pid: 30498 status null
- error spawnSync node ENOENT

== test spawnSync invalid cwd and `shell: true` ==
- pid: 30499 status null
- error spawnSync /bin/sh ENOENT
[v15.6.0 win32 x64]
v15.6.0 win32 x64

== test spawn with invalid command ==
- pid: undefined exitCode: null
- "error" event: spawn non-exist-command ENOENT
- nextTick pid: undefined exitCode: -4058
- after 100ms pid: undefined exitCode: -4058

== test spawn with invalid command and `shell: true` ==
- pid: 21860 exitCode: null
- nextTick pid: 21860 exitCode: null
- after 100ms pid: 21860 exitCode: 1

== test spawn with invalid cwd ==
- pid: undefined exitCode: null
- "error" event: spawn node ENOENT
- nextTick pid: undefined exitCode: -4058
- after 100ms pid: undefined exitCode: -4058

== test spawn invalid cwd and `shell: true` ==
- pid: undefined exitCode: null
- "error" event: spawn C:\Windows\system32\cmd.exe ENOENT
- nextTick pid: undefined exitCode: -4058
- after 100ms pid: undefined exitCode: -4058

== test spawnSync with invalid command ==
- pid: 0 status null
- error spawnSync non-exist-command ENOENT

== test spawnSync with invalid command and `shell: true` ==
- pid: 8864 status 1
- error undefined

== test spawnSync with invalid cwd ==
- pid: 0 status null
- error spawnSync node ENOENT

== test spawnSync invalid cwd and `shell: true` ==
- pid: 0 status null
- error spawnSync C:\Windows\system32\cmd.exe ENOENT

@aduh95
Copy link
Contributor

aduh95 commented Jan 22, 2021

Hum that's weird indeed, that might be a bug with spawnSync implementation. The "good news" is that the part of the docs you are modifying only covers the async version, the "bad news" is we now have to investigate for the pid in sync calls. Do you want to open an issue for the PID, and we can add a test in test/known_issues?

@dr-js
Copy link
Contributor Author

dr-js commented Jan 22, 2021

@aduh95 Sure, so in this PR I'll add docs & test checks for subProcess.pid, as it's only from the async version of spawn.

And I'll open another Issue with above test to discuss sync spawn pid behavior, and maybe a PR to add test to test/known_issues.

doc/api/child_process.md Outdated Show resolved Hide resolved
doc/api/child_process.md Outdated Show resolved Hide resolved
doc/api/child_process.md Outdated Show resolved Hide resolved
doc/api/child_process.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jan 31, 2021

LGTM with doc updates. @nodejs/documentation @nodejs/child_process

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2021
@nodejs-github-bot
Copy link
Collaborator

From the code `nodejs@8` and up should behave the same:
github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290

And a short test snippet:
```js
const { spawn } = require('child_process')

const subProcess = spawn('non-exist-command')
subProcess.on('error', (error) =>
	console.warn('mute Unhandled "error" event:', error))
console.log('- pid:', subProcess.pid)
process.nextTick(() => console.log('- after error emit'))
console.log('== end of test ==')
```

Note: the sync spawn result `pid` currently do not follow this pattern.

Co-authored-by: Rich Trott <[email protected]>

PR-URL: nodejs#37014
Reviewed-By: Antoine du Hamel <[email protected]>
Note: this only add checks for async spawn, as the sync spawn do not
return a `subProcess`.

PR-URL: nodejs#37014
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Feb 22, 2021

Landed in 574590d...1e0cfa3

@aduh95 aduh95 merged commit 1e0cfa3 into nodejs:master Feb 22, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
From the code `nodejs@8` and up should behave the same:
github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290

And a short test snippet:
```js
const { spawn } = require('child_process')

const subProcess = spawn('non-exist-command')
subProcess.on('error', (error) =>
	console.warn('mute Unhandled "error" event:', error))
console.log('- pid:', subProcess.pid)
process.nextTick(() => console.log('- after error emit'))
console.log('== end of test ==')
```

Note: the sync spawn result `pid` currently do not follow this pattern.

Co-authored-by: Rich Trott <[email protected]>

PR-URL: #37014
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Feb 28, 2021
Note: this only add checks for async spawn, as the sync spawn do not
return a `subProcess`.

PR-URL: #37014
Reviewed-By: Antoine du Hamel <[email protected]>
@dr-js dr-js deleted the doc-subprocess-pid branch March 4, 2021 13:18
targos pushed a commit that referenced this pull request May 1, 2021
From the code `nodejs@8` and up should behave the same:
github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290

And a short test snippet:
```js
const { spawn } = require('child_process')

const subProcess = spawn('non-exist-command')
subProcess.on('error', (error) =>
	console.warn('mute Unhandled "error" event:', error))
console.log('- pid:', subProcess.pid)
process.nextTick(() => console.log('- after error emit'))
console.log('== end of test ==')
```

Note: the sync spawn result `pid` currently do not follow this pattern.

Co-authored-by: Rich Trott <[email protected]>

PR-URL: #37014
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request May 1, 2021
Note: this only add checks for async spawn, as the sync spawn do not
return a `subProcess`.

PR-URL: #37014
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants