From 25ca9f177bb1208692b296748b3890d2ee15cb18 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 4 Dec 2018 12:39:30 +0100 Subject: [PATCH 1/2] src: dispose of V8 platform in `process.exit()` Calling `process.exit()` calls the C `exit()` function, which in turn calls the destructors of static C++ objects. This can lead to race conditions with other concurrently executing threads; disposing of all Worker threads and then the V8 platform instance helps with this (although it might not be a full solution for all problems of this kind). Refs: https://github.com/nodejs/node/issues/24403 Refs: https://github.com/nodejs/node/issues/25007 PR-URL: https://github.com/nodejs/node/pull/25061 Reviewed-By: Gireesh Punathil Reviewed-By: Joyee Cheung Reviewed-By: Ben Noordhuis --- src/env.cc | 7 +++++-- src/node.cc | 4 ++++ src/node_internals.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/env.cc b/src/env.cc index 8f061f9f83f288..3daf75e13043f6 100644 --- a/src/env.cc +++ b/src/env.cc @@ -673,10 +673,13 @@ void Environment::AsyncHooks::grow_async_ids_stack() { uv_key_t Environment::thread_local_env = {}; void Environment::Exit(int exit_code) { - if (is_main_thread()) + if (is_main_thread()) { + stop_sub_worker_contexts(); + DisposePlatform(); exit(exit_code); - else + } else { worker_context_->Exit(exit_code); + } } void Environment::stop_sub_worker_contexts() { diff --git a/src/node.cc b/src/node.cc index 9fb5ab3b8e1d3d..9390215b1b3065 100644 --- a/src/node.cc +++ b/src/node.cc @@ -383,6 +383,10 @@ static struct { #endif // !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR } v8_platform; +void DisposePlatform() { + v8_platform.Dispose(); +} + #ifdef __POSIX__ static const unsigned kMaxSignal = 32; #endif diff --git a/src/node_internals.h b/src/node_internals.h index 12089bc5f4af62..95b691884ec2b2 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -538,6 +538,8 @@ int ThreadPoolWork::CancelWork() { return uv_cancel(reinterpret_cast(&work_req_)); } +void DisposePlatform(); + static inline const char* errno_string(int errorno) { #define ERRNO_CASE(e) case e: return #e; switch (errorno) { From 49674f7983b0fa7ed8c1837d9a5234154715e24b Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Tue, 22 Jan 2019 13:44:33 +0100 Subject: [PATCH 2/2] src: ensure no more platform foreground tasks after Deinit Node first calls `Isolate::Dispose`, then `NodePlatform::UnregisterIsolate`. This again calls `PerIsolatePlatformData::Shutdown`, which (before this patch) called `FlushForegroundTasksInternal`, which might call `RunForegroundTask` if it finds foreground tasks to be executed. This will fail however, since `Isolate::GetCurrent` was already reset during `Isolate::Dispose`. Hence remove the check to `FlushForegroundTasksInternal` and add checks instead that no more foreground tasks are scheduled. Refs: https://github.com/v8/node/pull/86 PR-URL: https://github.com/nodejs/node/pull/25653 Reviewed-By: Gus Caplan Reviewed-By: Colin Ihrig --- src/node_platform.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_platform.cc b/src/node_platform.cc index fd8d045404ecc7..f6c122991ff967 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -259,7 +259,6 @@ void PerIsolatePlatformData::Shutdown() { if (flush_tasks_ == nullptr) return; - while (FlushForegroundTasksInternal()) {} CancelPendingDelayedTasks(); uv_close(reinterpret_cast(flush_tasks_),