lib: rearm pre-existing signal event registrations #24651

gireeshpunathil · 2018-11-26T06:47:07Z

process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. setupSignalHandlers perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence.

So rearm pre-existing signal event registrations to get those upto speed.

This is required by node-report; however given its independent existence
raising is a separate one.

I wish I could add a test for this, but realize it might not be possible, as the logic is exercised within the boot sequence that is not influenced / intercepted by any test cases.

Ref: #22712 (comment)

/cc @addaleax

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

nodejs-github-bot · 2018-11-26T06:47:09Z

@gireeshpunathil sadly an error occured when I tried to trigger a build :(

mscdex · 2018-11-26T13:28:54Z

lib/internal/process/main_thread_only.js

+  // re-arm pre-existing signal event registrations
+  // with this signal wrap capabilities.
+  const events = process.eventNames();
+  if (events != null) {


Won't this always be true? It should always be at the very least an empty array.

@mscdex - thanks, yes - the process object was subjected for events in the previous block, so there should be a non-null array. I removed the check now and tested, all good. PTAL.

mscdex · 2018-11-26T15:13:18Z

lib/internal/process/main_thread_only.js

+  // re-arm pre-existing signal event registrations
+  // with this signal wrap capabilities.
+  const events = process.eventNames();
+  events.forEach((ev) => {


minor nit: we can just do process.eventNames().forEach((ev) => {

@mscdex - done, and squashed. thanks.

process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: nodejs#22712 (comment)

mhdawson

LGTM

joyeecheung · 2018-11-27T12:19:26Z

lib/internal/process/main_thread_only.js

+
+  // re-arm pre-existing signal event registrations
+  // with this signal wrap capabilities.
+  process.eventNames().forEach((ev) => {


I thought we prefer for-of loops over forEach? (just a question)

@joyeecheung - thanks, I am unaware of any such preferences; pls let me know (or any links on) if there are any merits / tradeoffs between the two?

Just an observation, at least I thought we prefer for-of loops...
(on a side note, I took a look at the bytecode generated with for-of loops and they look humongous compared to forEach loops)

from perf standpoint, forEach is usually faster in newer versions of V8. from from a semantic standpoint i think it looks better to use forEach, but that's just opinion.

thanks @joyeecheung @devsnek for the reasoning; so I am keeping things as is.

gireeshpunathil · 2018-11-28T05:56:12Z

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

gireeshpunathil · 2018-11-28T15:32:05Z

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

Trott · 2018-11-28T18:34:04Z

Landed in 25ad8de

process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: nodejs#22712 (comment) PR-URL: nodejs#24651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: #22712 (comment) PR-URL: #24651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: nodejs#22712 (comment) PR-URL: nodejs#24651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

process.on('somesignal', ...) semantics expect the process to catch the signal and invoke the associated handler. `setupSignalHandlers` perform the additional task of preparing the libuv signal handler and associate it with the event handler. It is possible that by the time this is setup there could be pre-existing registrations that pre-date this setup in the boot sequence. So rearm pre-existing signal event registrations to get upto speed. Ref: #22712 (comment) PR-URL: #24651 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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

gireeshpunathil added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 26, 2018

gireeshpunathil requested review from joyeecheung and addaleax November 26, 2018 07:21

gireeshpunathil force-pushed the signal_rearm branch from 77bbb0d to 58fc8ec Compare November 26, 2018 07:24

gireeshpunathil mentioned this pull request Nov 26, 2018

node-report: meld into core #22712

Merged

4 tasks

addaleax approved these changes Nov 26, 2018

View reviewed changes

mscdex reviewed Nov 26, 2018

View reviewed changes

gireeshpunathil requested a review from mscdex November 26, 2018 14:52

mscdex reviewed Nov 26, 2018

View reviewed changes

gireeshpunathil force-pushed the signal_rearm branch from 2679a8a to 5f0cbdb Compare November 26, 2018 15:49

mhdawson approved these changes Nov 26, 2018

View reviewed changes

fhinkel approved these changes Nov 27, 2018

View reviewed changes

joyeecheung approved these changes Nov 27, 2018

View reviewed changes

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

Trott closed this Nov 28, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: rearm pre-existing signal event registrations #24651

lib: rearm pre-existing signal event registrations #24651

gireeshpunathil commented Nov 26, 2018

nodejs-github-bot commented Nov 26, 2018

mscdex Nov 26, 2018

gireeshpunathil Nov 26, 2018

mscdex Nov 26, 2018

gireeshpunathil Nov 26, 2018

mhdawson left a comment

joyeecheung Nov 27, 2018

gireeshpunathil Nov 27, 2018

joyeecheung Nov 27, 2018

devsnek Nov 27, 2018

gireeshpunathil Nov 28, 2018

gireeshpunathil commented Nov 28, 2018

gireeshpunathil commented Nov 28, 2018

Trott commented Nov 28, 2018

lib: rearm pre-existing signal event registrations #24651

lib: rearm pre-existing signal event registrations #24651

Conversation

gireeshpunathil commented Nov 26, 2018

Checklist

nodejs-github-bot commented Nov 26, 2018

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

mhdawson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Nov 28, 2018

gireeshpunathil commented Nov 28, 2018

Trott commented Nov 28, 2018