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: update spawnSync() status value possibilities #26680

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 15, 2019

The object returned by child_process.spawnSync() can have the status
property set to null if the process terminated due to a signal. We
even test for this in
test/parallel/test-child-process-spawnsync-kill-signal.js.

Update the documentation to reflect this.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

The object returned by `child_process.spawnSync()` can have the `status`
property set to `null` if the process terminated due to a signal. We
even test for this in
test/parallel/test-child-process-spawnsync-kill-signal.js.

Update the documentation to reflect this.
@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 Mar 15, 2019
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 15, 2019
@@ -868,7 +868,8 @@ changes:
* `output` {Array} Array of results from stdio output.
* `stdout` {Buffer|string} The contents of `output[1]`.
* `stderr` {Buffer|string} The contents of `output[2]`.
* `status` {number} The exit code of the child process.
* `status` {number|null} The exit code of the subprocess, or `null` if the
child process exited due to a signal.
Copy link
Contributor

Choose a reason for hiding this comment

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

"exited due to a signal" --> "terminated due to a signal" (if it terminated due to a signal, it didn't exit, which is why there is no exit code).

the signal docs below have the same problem, they should be changed to something like

The signal that terminated the child process, or null if the child process exited.

(or "if the child process terminated due to exiting.", but that is maybe too verbose).

Perhaps its obvious from the above, but you may want to emphasize that either status will be non-null, or signal will be non-null, but they will NEVER both have non-null values.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sam-github OK, I think I've addressed this. I went with end due to signal instead of terminate due to signal. PTAL.

@sam-github
Copy link
Contributor

https://nodejs.org/api/child_process.html#child_process_event_exit has decent text, might want to copy some of it.

https://nodejs.org/api/child_process.html#child_process_event_close has the same problem as spawnSync, it assumes the reader has already read and understood the exit event docs.

Searching the docs for "exit code" will find all kinds of references to "its an error when it is non-zero". This is technically correct, but subtle. You must understand that terminating by signal gives a code of null, which isn't zero.

Its unfortunate that spawnSync uses status for something that is called code most other places.

@Trott
Copy link
Member Author

Trott commented Mar 17, 2019

Its unfortunate that spawnSync uses status for something that is called code most other places.

Yes, unintentional API/terminology inconsistencies are going to become more and more of an issue over time. 😞

* `status` {number} The exit code of the child process.
* `signal` {string} The signal used to kill the child process.
* `status` {number|null} The exit code of the subprocess, or `null` if the
subprocess ended due to a signal.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't object to "ended", but please be aware that its non-standard terminology: streams end, processes terminate, on both posixy and windowsy operating systems, see:

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll switch to terminate since it's the best term for this.

@sam-github sam-github dismissed their stale review March 18, 2019 17:00

changes made

@Trott
Copy link
Member Author

Trott commented Mar 18, 2019

@Trott
Copy link
Member Author

Trott commented Mar 18, 2019

Landed in 5fc6c1d

@Trott Trott closed this Mar 18, 2019
Trott added a commit to Trott/io.js that referenced this pull request Mar 18, 2019
The object returned by `child_process.spawnSync()` can have the `status`
property set to `null` if the process terminated due to a signal. We
even test for this in
test/parallel/test-child-process-spawnsync-kill-signal.js.

Update the documentation to reflect this.

PR-URL: nodejs#26680
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
The object returned by `child_process.spawnSync()` can have the `status`
property set to `null` if the process terminated due to a signal. We
even test for this in
test/parallel/test-child-process-spawnsync-kill-signal.js.

Update the documentation to reflect this.

PR-URL: nodejs#26680
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
The object returned by `child_process.spawnSync()` can have the `status`
property set to `null` if the process terminated due to a signal. We
even test for this in
test/parallel/test-child-process-spawnsync-kill-signal.js.

Update the documentation to reflect this.

PR-URL: #26680
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
The object returned by `child_process.spawnSync()` can have the `status`
property set to `null` if the process terminated due to a signal. We
even test for this in
test/parallel/test-child-process-spawnsync-kill-signal.js.

Update the documentation to reflect this.

PR-URL: #26680
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
@Trott Trott deleted the null-status branch January 13, 2022 22:51
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.

9 participants