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

process: delay signal handler setup to pre-execution and skip them in workers #26227

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

process: setup signal handler in prepareMainThreadExecution

Because this is only necessary in the main thread.
Also removes the rearming of signal events since no
signal event handlers should be created during the execution
of node.js now that we've made the creation of stdout and stderr
streams lazy - this has been demonstrated in the test coverage.

process: move initialization of node-report into pre_execution.js

  • Splits signal handler setup code into two functions: one sets up
    process.on('SIGNAL_NAME'), another takes care of the signal
    triggers of node-report. Both should only happen on the main thread.
    The latter needs to happen after the node-report configurations
    are read into the process.
  • Move the initialization of node-report into pre_execution.js
    because it depends on CLI/environment settings.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Because this is only necessary in the main thread.
Also removes the rearming of signal events since no
signal event handlers should be created during the execution
of node.js now that we've made the creation of stdout and stderr
streams lazy - this has been demonstrated in the test coverage.
- Splits signal handler setup code into two functions: one sets up
  `process.on('SIGNAL_NAME')`, another takes care of the signal
  triggers of node-report. Both should only happen on the main thread.
  The latter needs to happen after the node-report configurations
  are read into the process.
- Move the initialization of node-report into pre_execution.js
  because it depends on CLI/environment settings.
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Feb 21, 2019
@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 21, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20926/ (:heavy_check_mark:)

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

Landed in 34470b0, b0869c6

@addaleax addaleax closed this Feb 24, 2019
addaleax pushed a commit that referenced this pull request Feb 24, 2019
Because this is only necessary in the main thread.
Also removes the rearming of signal events since no
signal event handlers should be created during the execution
of node.js now that we've made the creation of stdout and stderr
streams lazy - this has been demonstrated in the test coverage.

PR-URL: #26227
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 24, 2019
- Splits signal handler setup code into two functions: one sets up
  `process.on('SIGNAL_NAME')`, another takes care of the signal
  triggers of node-report. Both should only happen on the main thread.
  The latter needs to happen after the node-report configurations
  are read into the process.
- Move the initialization of node-report into pre_execution.js
  because it depends on CLI/environment settings.

PR-URL: #26227
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 25, 2019
Because this is only necessary in the main thread.
Also removes the rearming of signal events since no
signal event handlers should be created during the execution
of node.js now that we've made the creation of stdout and stderr
streams lazy - this has been demonstrated in the test coverage.

PR-URL: #26227
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 25, 2019
- Splits signal handler setup code into two functions: one sets up
  `process.on('SIGNAL_NAME')`, another takes care of the signal
  triggers of node-report. Both should only happen on the main thread.
  The latter needs to happen after the node-report configurations
  are read into the process.
- Move the initialization of node-report into pre_execution.js
  because it depends on CLI/environment settings.

PR-URL: #26227
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Because this is only necessary in the main thread.
Also removes the rearming of signal events since no
signal event handlers should be created during the execution
of node.js now that we've made the creation of stdout and stderr
streams lazy - this has been demonstrated in the test coverage.

PR-URL: #26227
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
- Splits signal handler setup code into two functions: one sets up
  `process.on('SIGNAL_NAME')`, another takes care of the signal
  triggers of node-report. Both should only happen on the main thread.
  The latter needs to happen after the node-report configurations
  are read into the process.
- Move the initialization of node-report into pre_execution.js
  because it depends on CLI/environment settings.

PR-URL: #26227
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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.

6 participants