lib: allow process kill by signal number #16944

sam-github · 2017-11-10T22:03:55Z

This brings the behaviour in line with the documentation.

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)

sam-github · 2017-11-10T22:05:47Z

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

addaleax · 2017-11-10T22:06:52Z

lib/internal/process.js

@@ -158,8 +158,8 @@ function setupKillAndExit() {
    }

    // preserve null signal
-    if (0 === sig) {
-      err = process._kill(pid, 0);
+    if (sig == (sig | 0)) {


Don’t you want to keep the ===?

cjihrig

LGTM if the CI is happy. It will need restarted, as the linter failed.

sam-github · 2017-11-10T22:28:11Z

Linting is fixed, build and passed locally, will wait for all the platforms to pass before restarting for an all-green run.

jasnell · 2017-11-12T02:04:53Z

test/parallel/test-process-kill-pid.js

+});
+
+assert.throws(function() { process.kill(1, 987); },
+              invalidSignalNumber);


this can be simplified to just

common.expectsError( () => process.kill(1, 987), { code: 'EINVAL', type: Error, message: 'kill EINVAL' } );

jasnell · 2017-11-12T18:39:24Z

test/parallel/test-process-kill-pid.js

-// Test that kill throws an error for invalid signal
-const unknownSignal = common.expectsError({
+// Test that kill throws an error for unknown signal names
+common.expectsError(process.kill(1, 'test'), {


common.expectsError( () => process.kill(1, 'test'), { ... }

That is, The first argument needs to be a function...

PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This brings the behaviour in line with the documentation. PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

sam-github · 2017-11-15T13:36:07Z

@jasnell passed locally, ci running

addaleax · 2017-11-18T20:00:32Z

@sam-github Was there a CI run associated with this?

CI: https://ci.nodejs.org/job/node-test-commit/14133/

addaleax · 2017-11-18T21:08:19Z

@sam-github Looks like this doesn’t work on Windows quite yet

bzoz · 2017-11-20T08:13:58Z

On Windows libuv first tries to open the process handle (src/win/process.c:1246) before checking for correct signal. If the signum is invalid, it will raise ENOSYS (src/win/process.c:1215). For this test to work, we would need to change libuv, which I guess would be rather simple.

sam-github · 2017-11-20T17:52:44Z

I'll take a look, should have time this week.

BridgeAR · 2018-02-01T10:23:15Z

@sam-github there seems to be an issue on Windows:

https://ci.nodejs.org/job/node-test-binary-windows/14617/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=0/console

not ok 314 parallel/test-process-kill-pid
  ---
  duration_ms: 0.297
  severity: fail
  stack: |-
    assert.js:72
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: 'ESRCH' strictEqual 'EINVAL'
        at Object.innerFn (c:\workspace\node-test-binary-windows\test\common\index.js:745:12)
        at expectedException (assert.js:382:19)
        at Function.throws (assert.js:427:16)
        at Object.expectsError (c:\workspace\node-test-binary-windows\test\common\index.js:784:12)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-process-kill-pid.js:68:8)
        at Module._compile (module.js:666:30)
        at Object.Module._extensions..js (module.js:677:10)
        at Module.load (module.js:578:32)
        at tryModuleLoad (module.js:518:12)
        at Function.Module._load (module.js:510:3)

bzoz · 2018-02-01T11:03:34Z

There is no process with PID 1 on Windows. Change those to 0 or process.pid and it should work.

BridgeAR · 2018-02-07T03:17:28Z

Ping @sam-github

BridgeAR · 2018-02-17T00:10:44Z

I pushed the fix that @bzoz suggested.

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

bzoz · 2018-02-17T09:27:15Z

CI failures on Windows are unrelated

BridgeAR · 2018-02-17T14:28:39Z

Running one more CI before landing.

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

BridgeAR · 2018-02-17T16:10:15Z

Landed in 7ca4ca8 🎉

This brings the behaviour in line with the documentation. PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

This brings the behaviour in line with the documentation. PR-URL: #16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

This brings the behaviour in line with the documentation. PR-URL: #16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

This brings the behaviour in line with the documentation. PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

MylesBorins · 2018-08-07T18:59:55Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Nov 10, 2017

addaleax reviewed Nov 10, 2017

View reviewed changes

sam-github force-pushed the kill-allow-numbers branch 2 times, most recently from f8fa77d to 823de05 Compare November 10, 2017 22:11

cjihrig approved these changes Nov 10, 2017

View reviewed changes

addaleax approved these changes Nov 10, 2017

View reviewed changes

jasnell reviewed Nov 12, 2017

View reviewed changes

sam-github force-pushed the kill-allow-numbers branch 2 times, most recently from 92235a0 to bfb2503 Compare November 12, 2017 13:20

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 12, 2017

jasnell reviewed Nov 12, 2017

View reviewed changes

sam-github force-pushed the kill-allow-numbers branch from bfb2503 to bb82ca2 Compare November 15, 2017 13:29

sam-github added 2 commits November 15, 2017 05:29

test: pass test function to expectsError

b0d675d

PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib: allow process kill by signal number

515f415

This brings the behaviour in line with the documentation. PR-URL: nodejs#16944 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

sam-github force-pushed the kill-allow-numbers branch from bb82ca2 to 515f415 Compare November 15, 2017 13:30

addaleax approved these changes Nov 18, 2017

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 18, 2017

bzoz mentioned this pull request Nov 21, 2017

win, process: EINVAL for invalid uv_kill signum libuv/libuv#1642

Closed

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018

bnoordhuis mentioned this pull request Feb 4, 2018

Can't send signals using signal numbers #18553

Closed

fixup test

f1a582a

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018

BridgeAR approved these changes Feb 17, 2018

View reviewed changes

BridgeAR closed this Feb 17, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

sam-github deleted the kill-allow-numbers branch October 16, 2018 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: allow process kill by signal number #16944

lib: allow process kill by signal number #16944

sam-github commented Nov 10, 2017 •

edited

Loading

sam-github commented Nov 10, 2017

addaleax Nov 10, 2017

cjihrig left a comment

sam-github commented Nov 10, 2017

jasnell Nov 12, 2017

sam-github Nov 12, 2017

jasnell Nov 12, 2017

sam-github commented Nov 15, 2017

addaleax commented Nov 18, 2017

addaleax commented Nov 18, 2017

bzoz commented Nov 20, 2017

sam-github commented Nov 20, 2017

BridgeAR commented Feb 1, 2018

bzoz commented Feb 1, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 17, 2018

bzoz commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

MylesBorins commented Aug 7, 2018

lib: allow process kill by signal number #16944

lib: allow process kill by signal number #16944

Conversation

sam-github commented Nov 10, 2017 • edited Loading

Checklist

Affected core subsystem(s)

sam-github commented Nov 10, 2017

addaleax Nov 10, 2017

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

sam-github commented Nov 10, 2017

jasnell Nov 12, 2017

Choose a reason for hiding this comment

sam-github Nov 12, 2017

Choose a reason for hiding this comment

jasnell Nov 12, 2017

Choose a reason for hiding this comment

sam-github commented Nov 15, 2017

addaleax commented Nov 18, 2017

addaleax commented Nov 18, 2017

bzoz commented Nov 20, 2017

sam-github commented Nov 20, 2017

BridgeAR commented Feb 1, 2018

bzoz commented Feb 1, 2018

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 17, 2018

bzoz commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

MylesBorins commented Aug 7, 2018

sam-github commented Nov 10, 2017 •

edited

Loading