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 setup of global exception handlers #26061

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Since bootstrap/node.js performs the setup synchronously,
the process exception handlers do not have to setup so early in
the bootstrap process - any fatal errors thrown before user code
execution should simply crash the process, and we do not care
about any clean up at that point. We don't care about emitting any
events if the process crash upon bootstrap either.

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

Since bootstrap/node.js performs the setup synchronously,
the process exception handlers do not have to setup so early in
the bootstrap process - any fatal errors thrown before user code
execution should simply crash the process, and we do not care
about any clean up at that point. We don't care about emitting any
events if the process crash upon bootstrap either.
@joyeecheung joyeecheung changed the title process: delay setup of execution handlers process: delay setup of global exception handlers Feb 12, 2019
process.hasUncaughtExceptionCaptureCallback =
hasUncaughtExceptionCaptureCallback;
}

// User-facing NODE_V8_COVERAGE environment variable that writes
Copy link
Member Author

@joyeecheung joyeecheung Feb 12, 2019

Choose a reason for hiding this comment

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

The higher-order bit is that everything below this point (plus setupTraceCategoryState() and process.argv[0] setup above) is environment-dependent and should be moved out of node.js - then it would be possible to snapshot Node.js after the execution of this script, which would be dedicated to setting up the process object and global proxy synchronously in an environment-independent manner.

@joyeecheung
Copy link
Member Author

@addaleax
Copy link
Member

Landed in 0eb71d4

@addaleax addaleax closed this Feb 15, 2019
addaleax pushed a commit that referenced this pull request Feb 15, 2019
Since bootstrap/node.js performs the setup synchronously,
the process exception handlers do not have to setup so early in
the bootstrap process - any fatal errors thrown before user code
execution should simply crash the process, and we do not care
about any clean up at that point. We don't care about emitting any
events if the process crash upon bootstrap either.

PR-URL: #26061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Gus Caplan <me@gus.host>
addaleax pushed a commit that referenced this pull request Feb 16, 2019
Since bootstrap/node.js performs the setup synchronously,
the process exception handlers do not have to setup so early in
the bootstrap process - any fatal errors thrown before user code
execution should simply crash the process, and we do not care
about any clean up at that point. We don't care about emitting any
events if the process crash upon bootstrap either.

PR-URL: #26061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Since bootstrap/node.js performs the setup synchronously,
the process exception handlers do not have to setup so early in
the bootstrap process - any fatal errors thrown before user code
execution should simply crash the process, and we do not care
about any clean up at that point. We don't care about emitting any
events if the process crash upon bootstrap either.

PR-URL: #26061
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Gus Caplan <me@gus.host>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants