GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: child_process: clarify `subprocess.pid` can be `undefined` when `ENOENT` by dr-js · Pull Request #37014 · nodejs/node

Conversation

@dr-js

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

Copy link
Copy Markdown
Contributor

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 ==')

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to Node.js documentation. labels Jan 21, 2021
@aduh95

aduh95 commented Jan 21, 2021

Copy link
Copy Markdown
Contributor

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

node/test/parallel/test-child-process-spawn-error.js

Lines 29 to 42 in 3df5afb

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-js

dr-js commented Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

Sure, I'll try update the test.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-js

dr-js commented Jan 22, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Jan 22, 2021

Copy link
Copy Markdown
Contributor

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-js

dr-js commented Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

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

aduh95 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Jan 24, 2021
View reviewed changes
Comment thread doc/api/child_process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@dr-js
dr-js force-pushed the doc-subprocess-pid branch from 9933250 to fd5748b Compare January 25, 2021 03:55
@dr-js dr-js mentioned this pull request Jan 25, 2021
child_process: sync spawn should not return pid number of a non-exist-command #37057
Closed
Trott
Trott reviewed Jan 31, 2021
View reviewed changes
Comment thread doc/api/child_process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Jan 31, 2021
View reviewed changes
Comment thread doc/api/child_process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Jan 31, 2021
View reviewed changes
Comment thread doc/api/child_process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Jan 31, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-js
dr-js force-pushed the doc-subprocess-pid branch from ec48ec2 to 61f86f3 Compare February 1, 2021 02:26
aduh95
aduh95 approved these changes Feb 19, 2021
View reviewed changes
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 19, 2021
@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aymen94
aymen94 approved these changes Feb 22, 2021
View reviewed changes
dr-js added 2 commits February 22, 2021 12:15
@dr-js @aduh95
doc,child_process: pid can be undefined when ENOENT
c1c98fb
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]>
@dr-js @aduh95
test,child_process: add check for subProcess.pid
1e0cfa3
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
aduh95 force-pushed the doc-subprocess-pid branch from 61f86f3 to 1e0cfa3 Compare February 22, 2021 11:16
@aduh95

aduh95 commented Feb 22, 2021

Copy link
Copy Markdown
Contributor

Landed in 574590d...1e0cfa3

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
aduh95 merged commit 1e0cfa3 into nodejs:master Feb 22, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
@dr-js @targos
doc,child_process: pid can be undefined when ENOENT
f20ce47
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
@dr-js @targos
test,child_process: add check for subProcess.pid
c554aa1
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]>
@targos targos mentioned this pull request Mar 2, 2021
v15.11.0 release proposal #37569
Merged
@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
@dr-js @targos
doc,child_process: pid can be undefined when ENOENT
a3c564b
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
@dr-js @targos
test,child_process: add check for subProcess.pid
51ef745
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
v14.17.0 proposal #38507
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jul 25, 2026
feat(node): bare-metal worker autoscaler ByteVeda/flexiq#542
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Aug 4, 2026
[CI] Web→Platform legal policy sync 자동화 AquilaXk/aquila-blog-web#10
Merged
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@aymen94 aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to Node.js documentation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@dr-js @aduh95 @Trott @nodejs-github-bot @aymen94
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.