From ed80c233cd87209758232f8ddc17ee99ef07f1d6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 19 Oct 2019 02:24:06 +0200 Subject: [PATCH] src: make implementing CancelPendingDelayedTasks for platform optional Fold `CancelPendingDelayedTasks()` into `UnregisterIsolate()` and make implementing it optional. It makes sense for these two operations to happen at the same time, so it is sufficient to provide a single operation instead of two separate ones. PR-URL: https://github.com/nodejs/node/pull/30034 Reviewed-By: Ben Noordhuis Reviewed-By: Shelley Vohr --- src/node.h | 9 ++++++-- src/node_main_instance.cc | 1 - src/node_platform.cc | 44 ++++++++++++++++++++++---------------- src/node_platform.h | 8 ++++--- src/node_v8_platform-inl.h | 5 ----- src/node_worker.cc | 2 -- 6 files changed, 38 insertions(+), 31 deletions(-) diff --git a/src/node.h b/src/node.h index d5c3c2a6671853..cae0673a37f80e 100644 --- a/src/node.h +++ b/src/node.h @@ -260,7 +260,11 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { // flushing. virtual bool FlushForegroundTasks(v8::Isolate* isolate) = 0; virtual void DrainTasks(v8::Isolate* isolate) = 0; - virtual void CancelPendingDelayedTasks(v8::Isolate* isolate) = 0; + + // TODO(addaleax): Remove this, it is unnecessary. + // This would currently be called before `UnregisterIsolate()` but will be + // folded into it in the future. + virtual void CancelPendingDelayedTasks(v8::Isolate* isolate); // This needs to be called between the calls to `Isolate::Allocate()` and // `Isolate::Initialize()`, so that initialization can already start @@ -270,7 +274,8 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { virtual void RegisterIsolate(v8::Isolate* isolate, struct uv_loop_s* loop) = 0; // This needs to be called right before calling `Isolate::Dispose()`. - // This function may only be called once per `Isolate`. + // This function may only be called once per `Isolate`, and discard any + // pending delayed tasks scheduled for that isolate. virtual void UnregisterIsolate(v8::Isolate* isolate) = 0; // The platform should call the passed function once all state associated // with the given isolate has been cleaned up. This can, but does not have to, diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index be53b585f0a106..0744063e0dc3a9 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -155,7 +155,6 @@ int NodeMainInstance::Run() { RunAtExit(env.get()); per_process::v8_platform.DrainVMTasks(isolate_); - per_process::v8_platform.CancelVMTasks(isolate_); #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); diff --git a/src/node_platform.cc b/src/node_platform.cc index f726382171488c..6e3b4abfee7287 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -285,22 +285,33 @@ void PerIsolatePlatformData::Shutdown() { // effectively deleting the tasks instead of running them. foreground_delayed_tasks_.PopAll(); foreground_tasks_.PopAll(); + scheduled_delayed_tasks_.clear(); - CancelPendingDelayedTasks(); - - ShutdownCbList* copy = new ShutdownCbList(std::move(shutdown_callbacks_)); - flush_tasks_->data = copy; + // Both destroying the scheduled_delayed_tasks_ lists and closing + // flush_tasks_ handle add tasks to the event loop. We keep a count of all + // non-closed handles, and when that reaches zero, we inform any shutdown + // callbacks that the platform is done as far as this Isolate is concerned. + self_reference_ = shared_from_this(); uv_close(reinterpret_cast(flush_tasks_), [](uv_handle_t* handle) { - std::unique_ptr callbacks( - static_cast(handle->data)); - for (const auto& callback : *callbacks) - callback.cb(callback.data); - delete reinterpret_cast(handle); + std::unique_ptr flush_tasks { + reinterpret_cast(handle) }; + PerIsolatePlatformData* platform_data = + static_cast(flush_tasks->data); + platform_data->DecreaseHandleCount(); + platform_data->self_reference_.reset(); }); flush_tasks_ = nullptr; } +void PerIsolatePlatformData::DecreaseHandleCount() { + CHECK_GE(uv_handle_count_, 1); + if (--uv_handle_count_ == 0) { + for (const auto& callback : shutdown_callbacks_) + callback.cb(callback.data); + } +} + NodePlatform::NodePlatform(int thread_pool_size, TracingController* tracing_controller) { if (tracing_controller) { @@ -382,10 +393,6 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { delayed->platform_data->DeleteFromScheduledTasks(delayed); } -void PerIsolatePlatformData::CancelPendingDelayedTasks() { - scheduled_delayed_tasks_.clear(); -} - void NodePlatform::DrainTasks(Isolate* isolate) { std::shared_ptr per_isolate = ForIsolate(isolate); @@ -409,12 +416,15 @@ bool PerIsolatePlatformData::FlushForegroundTasksInternal() { // the delay is non-zero. This should not be a problem in practice. uv_timer_start(&delayed->timer, RunForegroundTask, delay_millis, 0); uv_unref(reinterpret_cast(&delayed->timer)); + uv_handle_count_++; scheduled_delayed_tasks_.emplace_back(delayed.release(), [](DelayedTask* delayed) { uv_close(reinterpret_cast(&delayed->timer), [](uv_handle_t* handle) { - delete static_cast(handle->data); + std::unique_ptr task { + static_cast(handle->data) }; + task->platform_data->DecreaseHandleCount(); }); }); } @@ -454,10 +464,6 @@ bool NodePlatform::FlushForegroundTasks(Isolate* isolate) { return ForIsolate(isolate)->FlushForegroundTasksInternal(); } -void NodePlatform::CancelPendingDelayedTasks(Isolate* isolate) { - ForIsolate(isolate)->CancelPendingDelayedTasks(); -} - bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; } std::shared_ptr @@ -548,4 +554,6 @@ std::queue> TaskQueue::PopAll() { return result; } +void MultiIsolatePlatform::CancelPendingDelayedTasks(Isolate* isolate) {} + } // namespace node diff --git a/src/node_platform.h b/src/node_platform.h index d2eb325c12113e..24f7b337bb8fd7 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -78,12 +78,12 @@ class PerIsolatePlatformData : // posted during flushing of the queue are postponed until the next // flushing. bool FlushForegroundTasksInternal(); - void CancelPendingDelayedTasks(); const uv_loop_t* event_loop() const { return loop_; } private: void DeleteFromScheduledTasks(DelayedTask* task); + void DecreaseHandleCount(); static void FlushTasks(uv_async_t* handle); static void RunForegroundTask(std::unique_ptr task); @@ -95,6 +95,9 @@ class PerIsolatePlatformData : }; typedef std::vector ShutdownCbList; ShutdownCbList shutdown_callbacks_; + // shared_ptr to self to keep this object alive during shutdown. + std::shared_ptr self_reference_; + uint32_t uv_handle_count_ = 1; // 1 = flush_tasks_ uv_loop_t* const loop_; uv_async_t* flush_tasks_ = nullptr; @@ -102,7 +105,7 @@ class PerIsolatePlatformData : TaskQueue foreground_delayed_tasks_; // Use a custom deleter because libuv needs to close the handle first. - typedef std::unique_ptr> + typedef std::unique_ptr DelayedTaskPointer; std::vector scheduled_delayed_tasks_; }; @@ -137,7 +140,6 @@ class NodePlatform : public MultiIsolatePlatform { ~NodePlatform() override = default; void DrainTasks(v8::Isolate* isolate) override; - void CancelPendingDelayedTasks(v8::Isolate* isolate) override; void Shutdown(); // v8::Platform implementation. diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index e36f0a7d88bea4..ecd8f21d1a2653 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -113,10 +113,6 @@ struct V8Platform { platform_->DrainTasks(isolate); } - inline void CancelVMTasks(v8::Isolate* isolate) { - platform_->CancelPendingDelayedTasks(isolate); - } - inline void StartTracingAgent() { // Attach a new NodeTraceWriter only if this function hasn't been called // before. @@ -150,7 +146,6 @@ struct V8Platform { inline void Initialize(int thread_pool_size) {} inline void Dispose() {} inline void DrainVMTasks(v8::Isolate* isolate) {} - inline void CancelVMTasks(v8::Isolate* isolate) {} inline void StartTracingAgent() { if (!per_process::cli_options->trace_event_categories.empty()) { fprintf(stderr, diff --git a/src/node_worker.cc b/src/node_worker.cc index 20b162bd8135ba..025b5fed49cbb5 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -148,8 +148,6 @@ class WorkerThreadData { w_->isolate_ = nullptr; } - w_->platform_->CancelPendingDelayedTasks(isolate); - bool platform_finished = false; isolate_data_.reset();