child_process: exit spawnSync with null on signal #11288

cjihrig · 2017-02-10T15:03:41Z

This commit sets the spawnSync() exit code to null when the child is killed via signal. This brings the behavior more in sync with spawn().

Fixes: #11284

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

child_process

mscdex · 2017-02-10T16:17:30Z

src/spawn_sync.cc

+  if (exit_status_ >= 0) {
+    if (term_signal_ > 0)
+      js_result->Set(env()->status_string(), Null(env()->isolate()));
+    else


I know it didn't have them originally, but can we please add braces here (and around the if block for completeness), since the block spans more than one line?

jasnell

LGTM with @mscdex's nit addressed

evanlucas

LGTM with @mscdex's nit addressed

cjihrig · 2017-02-13T15:28:41Z

Nit addressed. CI: https://ci.nodejs.org/job/node-test-pull-request/6377/

EDIT: Unrelated failure. CI again: https://ci.nodejs.org/job/node-test-pull-request/6382/
EDIT: Unrelated again. CI: https://ci.nodejs.org/job/node-test-pull-request/6391/

sam-github · 2017-02-13T20:38:30Z

Personally, this looks like a bug fix to me, not semver-major, why should reporting an exit status of 0 for a child that did not exit be a feature?

Maybe it shouldn't get LTSed down to 4.x and 6.x (or maybe it should), but seems a good idea for 7.x.

cjihrig · 2017-02-13T23:06:30Z

I'm fine with this being a bug fix. I just labeled it semver major to be on the safe side.

I keep seeing build failures like this in the CI. I'm assuming they are unrelated.

jasnell · 2017-02-14T06:17:30Z

+1 for bugfix

This commit sets the spawnSync() exit code to null when the child is killed via signal. This brings the behavior more in sync with spawn(). Fixes: nodejs#11284 PR-URL: nodejs#11288 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

italoacasas · 2017-02-16T16:25:42Z

cc: @nodejs/collaborators

Just to confirm before moving this to staging. Are we sure about this not being semver-major?

thefourtheye · 2017-02-16T16:29:17Z

Bug fix sounds good to me.

This commit sets the spawnSync() exit code to null when the child is killed via signal. This brings the behavior more in sync with spawn(). Fixes: nodejs#11284 PR-URL: nodejs#11288 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

This commit sets the spawnSync() exit code to null when the child is killed via signal. This brings the behavior more in sync with spawn(). Fixes: #11284 PR-URL: #11288 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236)

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [nodejs#11489](nodejs#11489) PR-URL: nodejs#11553

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [nodejs#11489](nodejs#11489) * crypto: adding support for OPENSSL_CONF again (Sam Roberts) [nodejs#11006](nodejs#11006) * src: adding support for trace-event tracing (misterpoe) [nodejs#11106](nodejs#11106) PR-URL: nodejs#11553

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [#11288](nodejs/node#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [#11562](nodejs/node#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [#11275](nodejs/node#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [#11005](nodejs/node#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [#11236](nodejs/node#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [#11489](nodejs/node#11489) * crypto: adding support for OPENSSL_CONF again (Sam Roberts) [#11006](nodejs/node#11006) * src: adding support for trace-event tracing (misterpoe) [#11106](nodejs/node#11106) PR-URL: nodejs/node#11553 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

jasnell · 2017-03-07T17:18:38Z

This needs a backport PR to land on v4

This commit sets the spawnSync() exit code to null when the child is killed via signal. This brings the behavior more in sync with spawn(). Fixes: #11284 PR-URL: #11288 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) #10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) #10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) #11522 - `os.loadavg` is up to 151% faster. (Brian White) #11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) #11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) #10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) #10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) #10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) #10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) #11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) #8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) #9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) #10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) #10522 PR-URL: #11759

Notable changes * performance: The performance of several APIs has been improved. - `Buffer.compare()` is up to 35% faster on average. (Brian White) nodejs/node#10927 - `buffer.toJSON()` is up to 2859% faster on average. (Brian White) nodejs/node#10895 - `fs.*statSync()` functions are now up to 9.3% faster on average. (Brian White) nodejs/node#11522 - `os.loadavg` is up to 151% faster. (Brian White) nodejs/node#11516 - `process.memoryUsage()` is up to 34% faster. (Brian White) nodejs/node#11497 - `querystring.unescape()` for `Buffer`s is 15% faster on average. (Brian White) nodejs/node#10837 - `querystring.stringify()` is up to 7.8% faster on average. (Brian White) nodejs/node#10852 - `querystring.parse()` is up to 21% faster on average. (Brian White) nodejs/node#10874 * IPC: - Batched writes have been enabled for process IPC on platforms that support Unix Domain Sockets. (Alexey Orlenko) nodejs/node#10677 - Performance gains may be up to 40% for some workloads. * child_process: - `spawnSync` now returns a null `status` when child is terminated by a signal. (cjihrig) nodejs/node#11288 - This fixes the behavior to act like `spawn()` does. * http: - Control characters are now always rejected when using `http.request()`. (Ben Noordhuis) nodejs/node#8923 - Debug messages have been added for cases when headers contain invalid values. (Evan Lucas) nodejs/node#9195 * node: - Heap statistics now support values larger than 4GB. (Ben Noordhuis) nodejs/node#10186 * timers: - Timer callbacks now always maintain order when interacting with domain error handling. (John Barboza) nodejs/node#10522 PR-URL: nodejs/node#11759 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Feb 10, 2017

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2017

cjihrig mentioned this pull request Feb 10, 2017

child_process.spawnSync returns status=0 when signal=SIGKILL #11284

Closed

santigimeno approved these changes Feb 10, 2017

View reviewed changes

mscdex reviewed Feb 10, 2017

View reviewed changes

addaleax approved these changes Feb 10, 2017

View reviewed changes

jasnell approved these changes Feb 10, 2017

View reviewed changes

evanlucas approved these changes Feb 13, 2017

View reviewed changes

cjihrig force-pushed the spawnsync branch from 82a155e to ea7ceac Compare February 13, 2017 15:27

cjihrig force-pushed the spawnsync branch from ea7ceac to 63532dd Compare February 13, 2017 19:21

cjihrig removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 14, 2017

cjihrig force-pushed the spawnsync branch from 63532dd to 784eb2f Compare February 14, 2017 16:49

cjihrig merged commit 784eb2f into nodejs:master Feb 14, 2017

cjihrig deleted the spawnsync branch February 14, 2017 16:50

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added the dont-land-on-v4.x label Mar 7, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

igorklopov added a commit to vercel/pkg that referenced this pull request Apr 18, 2017

conform nodejs/node#11288

72b2a56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: exit spawnSync with null on signal #11288

child_process: exit spawnSync with null on signal #11288

cjihrig commented Feb 10, 2017

mscdex Feb 10, 2017

jasnell left a comment

evanlucas left a comment

cjihrig commented Feb 13, 2017 •

edited

Loading

sam-github commented Feb 13, 2017

cjihrig commented Feb 13, 2017

jasnell commented Feb 14, 2017

italoacasas commented Feb 16, 2017 •

edited

Loading

thefourtheye commented Feb 16, 2017

jasnell commented Mar 7, 2017

child_process: exit spawnSync with null on signal #11288

child_process: exit spawnSync with null on signal #11288

Conversation

cjihrig commented Feb 10, 2017

Checklist

Affected core subsystem(s)

mscdex Feb 10, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

evanlucas left a comment

Choose a reason for hiding this comment

cjihrig commented Feb 13, 2017 • edited Loading

sam-github commented Feb 13, 2017

cjihrig commented Feb 13, 2017

jasnell commented Feb 14, 2017

italoacasas commented Feb 16, 2017 • edited Loading

thefourtheye commented Feb 16, 2017

jasnell commented Mar 7, 2017

cjihrig commented Feb 13, 2017 •

edited

Loading

italoacasas commented Feb 16, 2017 •

edited

Loading