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

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

Closed
wants to merge 2 commits into from
Closed

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

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

@joyeecheung
process: setup signal handler in prepareMainThreadExecution
436800e 
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.
@joyeecheung
process: move initialization of node-report into pre_execution.js
9bcfc2d 
- 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
Copy link
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2679/pipeline

@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 •
edited by addaleax
Loading

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
@joyeecheung @addaleax
process: setup signal handler in prepareMainThreadExecution
34470b0 
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Feb 24, 2019
@joyeecheung @addaleax
process: move initialization of node-report into pre_execution.js
b0869c6 
- 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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Feb 25, 2019
@joyeecheung @addaleax
process: setup signal handler in prepareMainThreadExecution
a2a7adc 
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Feb 25, 2019
@joyeecheung @addaleax
process: move initialization of node-report into pre_execution.js
2d2143e 
- 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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@joyeecheung @rvagg
process: setup signal handler in prepareMainThreadExecution
57179a0 
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@joyeecheung @rvagg
process: move initialization of node-report into pre_execution.js
b1e739d 
- 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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @nodejs-github-bot @addaleax @jasnell @richardlau @BridgeAR