★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24651Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

lib: rearm pre-existing signal event registrations #24651

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@mscdex mscdex

@joyeecheung joyeecheung

@devsnek devsnek

@fhinkel fhinkel

@addaleax addaleax

@mhdawson mhdawson

Assignees
No one assigned
Labels
author ready lib / src process
Projects
None yet
Milestone
No milestone
9 participants
@gireeshpunathil @nodejs-github-bot @Trott @mscdex @fhinkel @addaleax @joyeecheung @devsnek @mhdawson
@gireeshpunathil
Copy link
Member

gireeshpunathil commented Nov 26, 2018

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

This comment has been minimized.

Sign in to view
Copy link

nodejs-github-bot commented Nov 26, 2018

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

@nodejs-github-bot nodejs-github-bot added the process label Nov 26, 2018

@gireeshpunathil gireeshpunathil added the lib / src label Nov 26, 2018

@gireeshpunathil gireeshpunathil requested review from joyeecheung and addaleax Nov 26, 2018

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:signal_rearm branch from 77bbb0d to 58fc8ec Nov 26, 2018

@gireeshpunathil gireeshpunathil referenced this pull request Nov 26, 2018

Merged

node-report: meld into core #22712

4 of 4 tasks complete
@addaleax

addaleax approved these changes Nov 26, 2018
View changes

@mscdex

mscdex reviewed Nov 26, 2018
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@mscdex

mscdex Nov 26, 2018

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@gireeshpunathil

gireeshpunathil Nov 26, 2018

Author Member

@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.

@gireeshpunathil gireeshpunathil requested a review from mscdex Nov 26, 2018

@mscdex

mscdex reviewed Nov 26, 2018
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@mscdex

mscdex Nov 26, 2018

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@gireeshpunathil

gireeshpunathil Nov 26, 2018

Author Member

@mscdex - done, and squashed. thanks.
@gireeshpunathil
lib: rearm pre-existing signal event registrations 
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)
Loading status checks…
5f0cbdb

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:signal_rearm branch from 2679a8a to 5f0cbdb Nov 26, 2018

@mhdawson

mhdawson approved these changes Nov 26, 2018
View changes

Copy link
Member

mhdawson left a comment

LGTM
@fhinkel

fhinkel approved these changes Nov 27, 2018
View changes

@joyeecheung

joyeecheung approved these changes Nov 27, 2018
View changes

lib/internal/process/main_thread_only.js

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

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Nov 27, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@gireeshpunathil

gireeshpunathil Nov 27, 2018

Author Member

@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?

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Nov 27, 2018

Member

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)

This comment has been minimized.

Sign in to view
Copy link
@devsnek

devsnek Nov 27, 2018

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@gireeshpunathil

gireeshpunathil Nov 28, 2018

Author Member

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

gireeshpunathil commented Nov 28, 2018

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

@Trott Trott added the author ready label Nov 28, 2018

@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member Author

gireeshpunathil commented Nov 28, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Nov 28, 2018

Landed in 25ad8de

@Trott Trott closed this Nov 28, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 28, 2018

@gireeshpunathil @Trott
lib: rearm pre-existing signal event registrations 
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 <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
25ad8de

targos added a commit that referenced this pull request Nov 29, 2018

@gireeshpunathil @targos
lib: rearm pre-existing signal event registrations 
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 <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
83ab5f4

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

This was referenced Dec 7, 2018

Closed
Closed
Closed
Closed
Closed

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

@gireeshpunathil @refack
lib: rearm pre-existing signal event registrations 
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 <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
6c17174

BethGriggs added a commit that referenced this pull request Feb 12, 2019

@gireeshpunathil @BethGriggs
lib: rearm pre-existing signal event registrations 
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 <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
Loading status checks…
8c93bd4

@BethGriggs BethGriggs referenced this pull request Feb 12, 2019

Merged

v10.15.3 proposal #26063

rvagg added a commit that referenced this pull request Feb 28, 2019

@gireeshpunathil @rvagg
lib: rearm pre-existing signal event registrations 
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 <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
54d847d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.