From 023ed84c64cbda7ccbb4f3fd8d570c00e223c3e2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 18 Feb 2020 20:17:15 +0100 Subject: [PATCH 1/2] src: discard tasks posted to platform TaskRunner during shutdown Discard tasks silently that are posted when the Isolate is being disposed. It is not possible to avoid a race condition window between unregistering the Isolate with the platform and disposing it in which background tasks and the Isolate deinit steps themselves may lead to new tasks being posted. The only sensible action in that case is discarding the tasks. Fixes: https://github.com/nodejs/node/issues/31752 Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104 Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548 Refs: https://github.com/nodejs/node/pull/31795 Refs: https://github.com/nodejs/node/pull/30909 --- src/node_platform.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/node_platform.cc b/src/node_platform.cc index 380a26ecb4bab6..713051efc3fc7a 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -244,14 +244,22 @@ void PerIsolatePlatformData::PostIdleTask(std::unique_ptr task) { } void PerIsolatePlatformData::PostTask(std::unique_ptr task) { - CHECK_NOT_NULL(flush_tasks_); + if (flush_tasks_ == nullptr) { + // V8 may post tasks during Isolate disposal. In that case, the only + // sensible path forward is to discard the task. + return; + } foreground_tasks_.Push(std::move(task)); uv_async_send(flush_tasks_); } void PerIsolatePlatformData::PostDelayedTask( std::unique_ptr task, double delay_in_seconds) { - CHECK_NOT_NULL(flush_tasks_); + if (flush_tasks_ == nullptr) { + // V8 may post tasks during Isolate disposal. In that case, the only + // sensible path forward is to discard the task. + return; + } std::unique_ptr delayed(new DelayedTask()); delayed->task = std::move(task); delayed->platform_data = shared_from_this(); From 06eff8055f9a53459a50d308f49519208121c6f7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 18 Feb 2020 20:21:45 +0100 Subject: [PATCH 2/2] Revert "src: keep main-thread Isolate attached to platform during Dispose" This reverts commit e460f8cf43863a5a8d73273ce311135ad3245699. It is no longer necessary after the previous commit, and restores consistency of the call order between the main thread code, the other call sites, and the documentation. Refs: https://github.com/nodejs/node/pull/31795 --- src/node_main_instance.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index d53eaa7329beed..ad6966cf4fc13e 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -102,12 +102,8 @@ NodeMainInstance::~NodeMainInstance() { if (!owns_isolate_) { return; } - // TODO(addaleax): Reverse the order of these calls. The fact that we first - // dispose the Isolate is a temporary workaround for - // https://github.com/nodejs/node/issues/31752 -- V8 should not be posting - // platform tasks during Dispose(), but it does in some WASM edge cases. - isolate_->Dispose(); platform_->UnregisterIsolate(isolate_); + isolate_->Dispose(); } int NodeMainInstance::Run() {