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.

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: move signal event handling into bootstrap/node.js #25859

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Moves the process.on() and promise.emit() calls happened during
bootstrap for signal events into bootstrap/node.js so it's easier
to tell the side effects.

Drive-by changes:

  • Moves the signal event re-arming to a point later during
    the bootstrap - as early as it were it's unlikely that there
    could be any existing signal events to re-arm for node-report.
  • Use a Map instead of an Object for signal wraps since it is used
    as a deletable dictionary anyway.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Moves the `process.on()` and `promise.emit()` calls happened during
bootstrap for signal events into `bootstrap/node.js` so it's easier
to tell the side effects.

Drive-by changes:

- Moves the signal event re-arming to a point later during
  the bootstrap - as early as it were it's unlikely that there
  could be any existing signal events to re-arm for node-report.
- Use a Map instead of an Object for signal wraps since it is used
  as a deletable dictionary anyway.
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jan 31, 2019
@joyeecheung
Copy link
Member Author

process.emit('newListener', ev);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Is there any specific reason for placing this block of code at this location? Why I ask is:

  • the re-arming logic was introduced to support report - though it supports any code block that registers signals while in the bootstrap process.
  • right now the signal handler setup is established just before the report is established, making the re-arming un-necessary, at least for report.
  • if we place this block of code as early as the process object's creation, we can eliminate the re-arming
  • if we place this block as late as towards the exit of setup, the re-arming will be leveraged by any one in the middle.
  • right now it seems to be a mix of both

and hence my question on the placement - is there any other considerations for this being here?

Code changes looks good to me.

Copy link
Member Author

@joyeecheung joyeecheung Feb 1, 2019

Choose a reason for hiding this comment

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

In fact I wonder if this is ever possible to trigger...as far as I can tell no signals events should be registered at this point (inside node.js) as now the stdio streams are lazily instantiated, and the branch has been yellow (never executed) in the coverage report

@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2019
@joyeecheung
Copy link
Member Author

Landed in 041424f

@joyeecheung joyeecheung closed this Feb 7, 2019
joyeecheung added a commit that referenced this pull request Feb 7, 2019
Moves the `process.on()` and `promise.emit()` calls happened during
bootstrap for signal events into `bootstrap/node.js` so it's easier
to tell the side effects.

Drive-by changes:

- Moves the signal event re-arming to a point later during
  the bootstrap - as early as it were it's unlikely that there
  could be any existing signal events to re-arm for node-report.
- Use a Map instead of an Object for signal wraps since it is used
  as a deletable dictionary anyway.

PR-URL: #25859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Feb 10, 2019
Moves the `process.on()` and `promise.emit()` calls happened during
bootstrap for signal events into `bootstrap/node.js` so it's easier
to tell the side effects.

Drive-by changes:

- Moves the signal event re-arming to a point later during
  the bootstrap - as early as it were it's unlikely that there
  could be any existing signal events to re-arm for node-report.
- Use a Map instead of an Object for signal wraps since it is used
  as a deletable dictionary anyway.

PR-URL: #25859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants