process: Send signal name to signal handlers #15606

robertrossmann · 2017-09-25T09:43:10Z

Checklist

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

Affected core subsystem(s)

process

Description

Allow the signal handler to receive the signal code that triggered the handler.

This is useful in situations where you have one common signal handler to gracefully exit a Node.js process (ie. one handler for SIGINT and SIGTERM) and you still want to know exactly which signal was used to stop the process.

Before

function shutdown(signal) {
  // Clean up resources
  console.log(`Quit - received ${signal}`)
}

process.once('SIGTERM', () => shutdown('SIGTERM'))
process.once('SIGINT', () => shutdown('SIGINT'))

After

function shutdown(signal) {
  // Clean up resources
  console.log(`Quit - received ${signal}`)
}

process.once('SIGTERM', shutdown)
process.once('SIGINT', shutdown)

bnoordhuis

Can you swap the callback arguments? Their current order makes this PR a needlessly backwards incompatible change.

That said, I'm -0 on this PR. It's mostly redundant and signal numbers are different across platforms. I don't see the point, TBH.

robertrossmann · 2017-09-25T10:47:45Z

@bnoordhuis Thanks for the feedback - I have swapped the arguments as requested.

One additional reason why this could be beneficial is that currently if you want to know the signal's code you have to look it up in the os.constants.signals table. Having the code an argument of the signal handler simplifies this process.

evanlucas · 2017-09-25T10:52:58Z

@robertrossmann thanks for taking the time to put together this PR. We have something similar in most of our services, so I can definitely appreciate the value in this. I'd be happy to help out to try and get the test working (at least on macOS and linux) as well

robertrossmann · 2017-09-25T11:12:49Z

Thanks @evanlucas for offering to help me with the tests. To describe current situation:

The test attaches a signal listener wrapped in common.mustCall() and in that listener function the arguments are checked to match expectations. Then, a signal is sent to the same process using process.kill(process.pid, 'SIGINT). However, the common.mustCall() wrapper throws an error stating that the listener was not called.

I wrote most of the test code by following test-signal-handler.js test. I am unsure if the new test file needs anything extra.

cjihrig · 2017-09-25T13:42:14Z

test/parallel/test-signal-args.js

+'use strict';
+
+const assert = require('assert');
+const common = require('../common');


Please move this up a line so that it is the first require().

cjihrig · 2017-09-25T13:42:29Z

test/parallel/test-signal-args.js

+const common = require('../common');
+const os = require('os');
+
+console.log(`process.pid: ${process.pid}`);


Please drop the console.log().

cjihrig · 2017-09-25T13:42:52Z

test/parallel/test-signal-args.js

+process.once('SIGINT', common.mustCall((signal, code) => {
+  assert.strictEqual(code, os.constants.signals[signal]);
+  assert.strictEqual(signal, 'SIGINT');
+}), 1);


You can drop the 1 here and below, as that is the default value.

cjihrig · 2017-09-25T13:45:17Z

doc/api/process.md

+process.on('SIGINT', handle);
+process.on('SIGTERM', handle);
+
+function handle(signal, code) {


I think the documentation needs to mention that these new arguments are provided.

I am sorry but why is the signal added as well? The handler itself already listens to the signal and that is redundant therefore?

robertrossmann · 2017-09-25T14:59:36Z

@cjihrig Thank you for your feedback - I have updated the code as requested and added extra sentence to the docs about the function arguments.

cjihrig

Code LGTM, but like @bnoordhuis I question whether or not we really need it.

cjihrig · 2017-09-25T15:05:29Z

doc/api/process.md

@@ -350,6 +350,9 @@ Signal events will be emitted when the Node.js process receives a signal. Please
 refer to signal(7) for a listing of standard POSIX signal names such as
 `SIGINT`, `SIGHUP`, etc.

+The signal handler will receive two arguments: the signal's name (`'SIGINT'`,
+ `'SIGTERM'` etc.) and the signal's numeric code, (`2`, `15`, respectively).


Missing a comma after SIGTERM.

numeric code, (2, 15, respectively) -> numeric code, respectively.

Fixed, thanks. I preserved the numeric examples, though - let me know if you intended to have them removed.

I did mean to remove them. I don't think they add much value. If you're going to keep them, please throw an , etc. in there too.

Fishrock123 · 2017-09-25T19:37:38Z

Sorry for being pedantic but, can't this just be put in a module that maps separate handlers with their appropriate info and then does the dispatch?

If that already exists, does it not work well?

robertrossmann · 2017-09-26T09:16:57Z

@Fishrock123 You are correct that the functionality included in this PR can easily be achieved by other means (see my initial example). It is very easy to wrap a signal handler in an arrow function and pass the signal name to the actual handler, and it is equally easy to look up that signal's code from os.constants.signals.

This PR is just an attempt to improve the ergonomics of such task by not requiring the developer to do these extra steps when Node.js already can provide all the information necessary.

So, yes - Node.js does not need this - it does not introduce any new features or functionality. It just attempts to improve the way people work with signal handlers.

comments were addressed

BridgeAR · 2017-09-28T05:42:11Z

I am -0 on this if this should land I would prefer to land it without the redundancy of the signal.

robertrossmann · 2017-09-28T06:07:05Z

@BridgeAR The signal is given to the handler so that the handler knows which signal triggered it. This is useful in situations where the same handler is listening on multiple signals and you want to know which signal caused the handler to be triggered.

As I mentioned earlier, the whole point of this PR is merely ergonomics - I can still achieve my goal using other means (arrow functions, looking up the signal code elsewhere etc.).

bnoordhuis

My apologies, I asked you to swap the callback arguments because I thought listeners already received the signal name as their first argument, but they don't.

I'm still not convinced that the signal number (the second argument) is all that useful.

It's redundant and might turn into steady source of bugs if programmers assume that because SIGFOO is 42 on platforms A and B, it's also 42 on C and D, and then find out the hard way when it's not.

bnoordhuis · 2017-09-28T07:22:24Z

doc/api/process.md

@@ -350,6 +350,9 @@ Signal events will be emitted when the Node.js process receives a signal. Please
 refer to signal(7) for a listing of standard POSIX signal names such as
 `SIGINT`, `SIGHUP`, etc.

+The signal handler will receive two arguments: the signal's name (`'SIGINT'`,
+ `'SIGTERM'`, etc.) and the signal's numeric code.


Maybe move this below the next paragraph or merge them. I think an unwitting reader would assume the second one is a continuation of the first, i.e., that it's about the callback's arguments.

Fishrock123 · 2017-09-28T18:15:55Z

I think I'm ok with this, but I do also prefer @bnoordhuis's recommendation to not pass the signal number.

If the signal number is necessary, it can be retrieved from os.constants.signals.

robertrossmann · 2017-10-05T13:34:26Z

I have rebased on current master and hopefully addressed all review feedback. I have also fixed the tests 🎉:

https://github.com/nodejs/node/pull/15606/files#diff-05a7b1950b62d59da8caf56feafde636R20

bnoordhuis

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

robertrossmann · 2017-10-16T07:53:13Z

It seems the test suite got stuck somewhere. 🤔 Can someone look at it and perhaps restart it? Thank you!

PS. Should I rebase once again or is it okay if this is based off of some older commit?

gireeshpunathil · 2017-10-16T08:17:20Z

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

robertrossmann · 2017-10-16T09:23:43Z

Ooops, looks like I forgot to run make lint before pushing and there were 2 linter errors being reported. Not sure why the linter CI did not report the build as failed and instead stalled - when I looked at the job report there were clearly errors and the build was reported as failed. Looking at some other jobs it seems the test suite passed at least for some targets even though the status was not reported back to this PR. 🤔

I have fixed all linter errors, rebased again from master and made sure the test suite still passes locally. Apologies for the hurdle!

joyeecheung · 2017-10-16T10:09:11Z

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

robertrossmann · 2017-10-16T17:08:04Z

Looks like the CI did not properly report back to this PR - I randomly checked some of the tasks and they are listed as successful on CI. Just FYI.

Fishrock123

Nothing blocking imo but a small fix

Fishrock123 · 2017-10-16T17:19:33Z

test/parallel/test-signal-args.js

+
+// Prevent Node.js from exiting due to empty event loop before signal handlers
+// are fired
+setTimeout(() => {}, 0);


setImmediate would be better. Note also that 0 coerces to 1 in timers durations.

Fishrock123

Then again, the test is failing on Windows. https://ci.nodejs.org/job/node-test-binary-windows/11940/

Fishrock123 · 2017-10-16T17:22:59Z

The CI isn't outputting a stack for some reason:

not ok 336 parallel/test-signal-args
  ---
  duration_ms: 0.120
  severity: fail
  stack: |-
  ...

Maybe someone from @nodejs/platform-windows can run the test on a local machine?

robertrossmann · 2017-10-16T17:38:17Z

@Fishrock123 Thanks for the suggestion, I updated the code.

About windows failure, from the docs:

Note: Windows does not support sending signals, but Node.js offers some emulation with process.kill(), and subprocess.kill(). Sending signal 0 can be used to test for the existence of a process. Sending SIGINT, SIGTERM, and SIGKILL cause the unconditional termination of the target process.

Could it be that the windows process is being unconditionally terminated due to it receiving SIGINT or SIGTERM? If that's the case, it would explain why there is no stack trace.

There is a mention of some signal emulation on Windows but I have no experience with win32 systems. Just a wild guess here.

robertrossmann · 2017-10-31T15:08:50Z

Is there anything I can do to have this merged? There is only one pending review from @Fishrock123 which has been addressed (win32 failures). Thank you!

Windows issues appear to be fixed.

jasnell · 2017-11-02T05:25:41Z

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

BridgeAR · 2017-11-22T18:44:14Z

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

jasnell · 2017-11-22T22:00:40Z

This is looking good, just waiting for the last few CI bots to finish

PR-URL: #15606 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

jasnell · 2017-11-22T22:42:03Z

Landed in 9b2cf1c

PR-URL: #15606 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: Coming Soon

Notable changes: * async\_hooks: - add trace events to async_hooks (Andreas Madsen) #15538 - add provider types for net server (Andreas Madsen) #17157 * console: - console.debug can now be used outside of the inspector (Benjamin Zaslavsky) #17033 * deps: - upgrade libuv to 1.18.0 (cjihrig) #17282 - patch V8 to 6.2.414.46 (Myles Borins) #17206 * module: - module.builtinModules will return a list of built in modules (Jon Moss) #16386 * n-api: - add helper for addons to get the event loop (Anna Henningsen) #17109 * process: - process.setUncaughtExceptionCaptureCallback can now be used to customize behavior for `--abort-on-uncaught-exception` (Anna Henningsen) #17159 - A signal handler is now able to receive the signal code that triggered the handler. (Robert Rossmann) #15606 * src: - embedders can now use Node::CreatePlatform to create an instance of NodePlatform (Cheng Zhao) #16981 * stream: - writable.writableHighWaterMark and readable.readableHighWaterMark will return the values the stream object was instantiated with (Calvin Metcalf) #12860 * **Added new collaborators** * [maclover7](https://github.com/maclover7) Jon Moss * [guybedford](https://github.com/guybedford) Guy Bedford * [hashseed](https://github.com/hashseed) Yang Guo PR-URL: #17631

nodejs/node#15606

* SignalsListener signal name nodejs/node#15606 * created Node 8 folder * Create tsconfig.json * Create tslint.json * Create node-tests.ts * Node 9 * update node header * Tabs to spaces * copy inspector.d.ts to v8 folder * correct path mapping for v8 * disable no-declare-current-package for v8 module * Correct version to 9.3.x Add writableHighWaterMark/readableHighWaterMark to WriteStream/ReadStream fixed setUncaughtExceptionCaptureCallback() Add module.builtinModules change os.EOL to const add writableHighWaterMark to Duplex writableHighWaterMark and readableHighWaterMark are readonly * fs.realpathSync.native and fs.realpath.native * fs.realpath.native tests

MylesBorins · 2018-08-02T14:34:49Z

Is this something we would want to backport to 8.x?

PR-URL: nodejs#15606 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

Backport-PR-URL: #22380 PR-URL: #15606 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

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

bnoordhuis previously requested changes Sep 25, 2017

View reviewed changes

cjihrig requested changes Sep 25, 2017

View reviewed changes

cjihrig approved these changes Sep 25, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

bnoordhuis reviewed Sep 28, 2017

View reviewed changes

bnoordhuis approved these changes Oct 5, 2017

View reviewed changes

evanlucas approved these changes Oct 5, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 16, 2017

View reviewed changes

robertrossmann changed the title ~~process: Send signal code, name to signal handlers~~ process: Send signal name to signal handlers Oct 16, 2017

Fishrock123 approved these changes Oct 16, 2017

View reviewed changes

Fishrock123 previously requested changes Oct 16, 2017

View reviewed changes

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

jasnell closed this Nov 22, 2017

robertrossmann deleted the process-signal-args branch November 22, 2017 22:49

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

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

OwnageIsMagic added a commit to OwnageIsMagic/DefinitelyTyped that referenced this pull request Dec 31, 2017

SignalsListener signal name

d78a401

nodejs/node#15606

OwnageIsMagic mentioned this pull request Dec 31, 2017

Node 9.3 DefinitelyTyped/DefinitelyTyped#22593

Merged

8 tasks

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

process: Send signal name to signal handlers #15606

process: Send signal name to signal handlers #15606

Conversation

robertrossmann commented Sep 25, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Description

Before

After

bnoordhuis left a comment

Choose a reason for hiding this comment

robertrossmann commented Sep 25, 2017

evanlucas commented Sep 25, 2017

robertrossmann commented Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrossmann commented Sep 25, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Sep 25, 2017

robertrossmann commented Sep 26, 2017

BridgeAR commented Sep 28, 2017

robertrossmann commented Sep 28, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Sep 28, 2017

robertrossmann commented Oct 5, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

robertrossmann commented Oct 16, 2017

gireeshpunathil commented Oct 16, 2017

robertrossmann commented Oct 16, 2017 • edited Loading

joyeecheung commented Oct 16, 2017

robertrossmann commented Oct 16, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment • edited Loading

Choose a reason for hiding this comment

Fishrock123 commented Oct 16, 2017

robertrossmann commented Oct 16, 2017

robertrossmann commented Oct 31, 2017

jasnell commented Nov 2, 2017

BridgeAR commented Nov 22, 2017

jasnell commented Nov 22, 2017

jasnell commented Nov 22, 2017

MylesBorins commented Aug 2, 2018

robertrossmann commented Sep 25, 2017 •

edited

Loading

robertrossmann commented Sep 25, 2017 •

edited

Loading

robertrossmann commented Oct 16, 2017 •

edited

Loading

Fishrock123 left a comment •

edited

Loading