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.

Sign up for GitHub

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

Jump to bottom

src: add SIGINFO to supported signals #6093

Closed
wants to merge 1 commit into from
Closed

src: add SIGINFO to supported signals #6093

wants to merge 1 commit into from

Conversation

jamesreggio
Copy link
Contributor

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

Exposed through process module, change in constants within src.

Description of change

Though not a POSIX signal, SIGINFO is supported by BSD systems (including Mac OS X) and is amongst the few signals that can be triggered in a terminal via a simple key combination (CTRL-T).

Supporting SIGINFO on these systems will enable the use case where long-running terminal applications can report their status upon receiving the signal, and the signal can be triggered with a keystroke, instead of requiring the use of kill.

A brief examination of src/node_constants.cc indicates that there's no philosophical decision to support only POSIX signals, so I assume SIGINFO was an oversight. (SIGPWR, SIGLOST, and SIGWINCH are all present and non-POSIX). It's worth noting that on Linux, SIGINFO is an alias for SIGPWR, which is why I employed defensive conditionals in src/node.cc.

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Apr 7, 2016
bnoordhuis
bnoordhuis reviewed Apr 7, 2016
View reviewed changes
src/node.cc
@@ -704,6 +704,12 @@ const char *signo_string(int signo) {
# endif
#endif

#ifdef SIGINFO
# if SIGINFO != SIGPWR
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be # if defined(SIGPWR) && SIGINFO != SIGPWR - SIGINFO does not imply the existence of SIGPWR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That defense is not necessary — the C/C++ preprocessors evaluate all undefined symbols to 0. See here.

It's worth noting that the code immediately preceding this (Lines 689-705) employs the same bare #if where the RHS may be undefined. Specific example: SIGPWR is defined on Linux, but not SIGLOST (Source).

Copy link
Member

Choose a reason for hiding this comment

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

That defense is not necessary — the C/C++ preprocessors evaluate all undefined symbols to 0.

It emits nasty warnings, however.

SIGPWR is defined on Linux, but not SIGLOST

Perhaps you mean it's not documented but the define itself exists, it's identical to SIGPWR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to defer to you on this, but I do want to clarify that I gave thought to this initially and determined that Node is not being built with -Wundef, which would print the nasty warnings you mentioned.

(In fact, V8 relies heavily upon undefined symbols, and adding -Wundef to the cflags defined within common.gypi creates thousands of warnings throughout both the V8 and the Node source.)

I'll update the branch momentarily.

@jamesreggio
src: add SIGINFO to supported signals
fb97403 
Though not a POSIX signal, SIGINFO is supported by BSD systems
(including Mac OS X) and is amongst the few signals that can be
triggered in a terminal via a simple key combination (CTRL-T).

On Linux, SIGINFO is an alias for SIGPWR; hence the defensive
conditionals in src/node.cc.
@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

/cc @bnoordhuis

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 7, 2016
@jamesreggio
Copy link
Contributor Author

@bnoordhuis, does my revision (reflected in fb97403) address your concerns?

@bnoordhuis
Copy link
Member

LGTM. CI, just in case: https://ci.nodejs.org/job/node-test-pull-request/2251/

@jamesreggio
Copy link
Contributor Author

Thanks @bnoordhuis. The failure looks spurious, but would you please confirm.

@jasnell
Copy link
Member

jasnell commented Apr 13, 2016

LGTM

@bnoordhuis
Copy link
Member

Thanks James, landed in fb5f66a. I'll tag this for v5.x.

@bnoordhuis bnoordhuis closed this Apr 14, 2016
bnoordhuis pushed a commit that referenced this pull request Apr 14, 2016
@jamesreggio @bnoordhuis
src: add SIGINFO to supported signals
fb5f66a 
Though not a POSIX signal, SIGINFO is supported by BSD systems
(including Mac OS X) and is amongst the few signals that can be
triggered in a terminal via a simple key combination (CTRL-T).

On Linux, SIGINFO is an alias for SIGPWR; hence the defensive
conditionals in src/node.cc.

PR-URL: #6093
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jamesreggio jamesreggio deleted the add-siginfo-const branch April 15, 2016 15:32
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@jamesreggio
src: add SIGINFO to supported signals
7d54d85 
Though not a POSIX signal, SIGINFO is supported by BSD systems
(including Mac OS X) and is amongst the few signals that can be
triggered in a terminal via a simple key combination (CTRL-T).

On Linux, SIGINFO is an alias for SIGPWR; hence the defensive
conditionals in src/node.cc.

PR-URL: #6093
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
e43735b 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
This was referenced Apr 21, 2016
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@jamesreggio @MylesBorins
src: add SIGINFO to supported signals
9f2dc70 
Though not a POSIX signal, SIGINFO is supported by BSD systems
(including Mac OS X) and is amongst the few signals that can be
triggered in a terminal via a simple key combination (CTRL-T).

On Linux, SIGINFO is an alias for SIGPWR; hence the defensive
conditionals in src/node.cc.

PR-URL: #6093
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@MylesBorins
2016-04-21, Version 5.11.0 (Stable) Release
8dc1d5e 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behavior was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
0c5ab5c 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
1fa8fce 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@joelostrowski
2016-04-20, Version 5.11.0 (Stable) Release
002d57b 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) nodejs#5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) nodejs#6279
  * update ESLint to 2.7.0
    (silverwind) nodejs#6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) nodejs#6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) nodejs#6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) nodejs#6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) nodejs#6090

src:
  * add SIGINFO to supported signals
    (James Reggio) nodejs#6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) nodejs#6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) nodejs#6069

PR-URL: nodejs#6322
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@jamesreggio @jasnell
src: add SIGINFO to supported signals
6e76419 
Though not a POSIX signal, SIGINFO is supported by BSD systems
(including Mac OS X) and is amongst the few signals that can be
triggered in a terminal via a simple key combination (CTRL-T).

On Linux, SIGINFO is an alias for SIGPWR; hence the defensive
conditionals in src/node.cc.

PR-URL: #6093
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@jasnell
2016-04-20, Version 5.11.0 (Stable) Release
c764c44 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jamesreggio @jasnell @bnoordhuis @mscdex