-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
src: fix race on process exit and off thread CA loading #59632
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
base: main
Are you sure you want to change the base?
Conversation
When calling `process.exit()` or on uncaught exceptions as soon as the process starts, the process will try to terminate immediately. In this case, there could be a race condition on the unfinished off-thread system CA loader which tries to access the OpenSSL API which has been de-inited on the main thread.
3747069
to
f02900e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though I have some comments, can be done as follow up though
@@ -1004,6 +1007,11 @@ void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code) { | |||
// in node_v8_platform-inl.h | |||
uv_library_shutdown(); | |||
DisposePlatform(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that in the normal exit path, this is done separately and guarded by kNoInitializeNodeV8Platform
Lines 1312 to 1321 in ca76b39
if (!(flags & ProcessInitializationFlags::kNoInitializeNodeV8Platform)) { | |
V8::DisposePlatform(); | |
// uv_run cannot be called from the time before the beforeExit callback | |
// runs until the program exits unless the event loop has any referenced | |
// handles after beforeExit terminates. This prevents unrefed timers | |
// that happen to terminate during shutdown from being run unsafely. | |
// Since uv_run cannot be called, uv_async handles held by the platform | |
// will never be fully cleaned up. | |
per_process::v8_platform.Dispose(); | |
} |
Maybe we should wrap the things that need to be done both on normal and abnormal exit into a helper and call them in both DefaultProcessExitHandlerInternal and TearDownOncePerProcess?
(Also, from what I can tell, this function would only get called on the main thread - it would be better to assert it, most things in this section can only be called once by one thread)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59632 +/- ##
==========================================
- Coverage 89.85% 89.83% -0.02%
==========================================
Files 667 667
Lines 196260 196261 +1
Branches 38559 38562 +3
==========================================
- Hits 176341 176319 -22
- Misses 12368 12376 +8
- Partials 7551 7566 +15
🚀 New features to boost your workflow:
|
When calling
process.exit()
or on uncaught exceptions as soon as theprocess starts, the process will try to terminate immediately. In this
case, there could be a race condition on the unfinished off-thread
system CA loader which tries to access the OpenSSL API which has been
de-inited on the main thread.
Example stacks:
Refs: #59550