From 8522e2420da41791a6fa64557039cc66ef27264e Mon Sep 17 00:00:00 2001 From: Franziska Hinkelmann Date: Sun, 12 Nov 2017 16:23:31 +0100 Subject: [PATCH] src: use unique_ptr in platform implementation Replace raw pointers in task queues with std::unique_ptr. This makes ownership obvious. PR-URL: https://github.com/nodejs/node/pull/16970 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil --- src/node_platform.cc | 68 ++++++++++++++++++++++---------------------- src/node_platform.h | 17 +++++------ 2 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/node_platform.cc b/src/node_platform.cc index 7cec43cbf44509..51927cf7f9f792 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -18,9 +18,8 @@ using v8::TracingController; static void BackgroundRunner(void* data) { TaskQueue* background_tasks = static_cast*>(data); - while (Task* task = background_tasks->BlockingPop()) { + while (std::unique_ptr task = background_tasks->BlockingPop()) { task->Run(); - delete task; background_tasks->NotifyOfCompletion(); } } @@ -39,18 +38,19 @@ void PerIsolatePlatformData::FlushTasks(uv_async_t* handle) { platform_data->FlushForegroundTasksInternal(); } -void PerIsolatePlatformData::CallOnForegroundThread(Task* task) { - foreground_tasks_.Push(task); +void PerIsolatePlatformData::CallOnForegroundThread( + std::unique_ptr task) { + foreground_tasks_.Push(std::move(task)); uv_async_send(flush_tasks_); } void PerIsolatePlatformData::CallDelayedOnForegroundThread( - Task* task, double delay_in_seconds) { - auto delayed = new DelayedTask(); - delayed->task = task; + std::unique_ptr task, double delay_in_seconds) { + std::unique_ptr delayed(new DelayedTask()); + delayed->task = std::move(task); delayed->platform_data = this; delayed->timeout = delay_in_seconds; - foreground_delayed_tasks_.Push(delayed); + foreground_delayed_tasks_.Push(std::move(delayed)); uv_async_send(flush_tasks_); } @@ -125,14 +125,13 @@ size_t NodePlatform::NumberOfAvailableBackgroundThreads() { return threads_.size(); } -void PerIsolatePlatformData::RunForegroundTask(Task* task) { +void PerIsolatePlatformData::RunForegroundTask(std::unique_ptr task) { Isolate* isolate = Isolate::GetCurrent(); HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); InternalCallbackScope cb_scope(env, Local(), { 0, 0 }, InternalCallbackScope::kAllowEmptyResource); task->Run(); - delete task; } void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { @@ -141,7 +140,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { auto it = std::find(tasklist.begin(), tasklist.end(), delayed); CHECK_NE(it, tasklist.end()); tasklist.erase(it); - RunForegroundTask(delayed->task); + RunForegroundTask(std::move(delayed->task)); uv_close(reinterpret_cast(&delayed->timer), [](uv_handle_t* handle) { delete static_cast(handle->data); @@ -162,10 +161,10 @@ void NodePlatform::DrainBackgroundTasks(Isolate* isolate) { PerIsolatePlatformData* per_isolate = ForIsolate(isolate); do { - // Right now, there is no way to drain only background tasks associated with - // a specific isolate, so this sometimes does more work than necessary. - // In the long run, that functionality is probably going to be available - // anyway, though. + // Right now, there is no way to drain only background tasks associated + // with a specific isolate, so this sometimes does more work than + // necessary. In the long run, that functionality is probably going to + // be available anyway, though. background_tasks_.BlockingDrain(); } while (per_isolate->FlushForegroundTasksInternal()); } @@ -173,28 +172,29 @@ void NodePlatform::DrainBackgroundTasks(Isolate* isolate) { bool PerIsolatePlatformData::FlushForegroundTasksInternal() { bool did_work = false; - while (auto delayed = foreground_delayed_tasks_.Pop()) { + while (std::unique_ptr delayed = + foreground_delayed_tasks_.Pop()) { did_work = true; uint64_t delay_millis = static_cast(delayed->timeout + 0.5) * 1000; - delayed->timer.data = static_cast(delayed); + delayed->timer.data = static_cast(delayed.get()); uv_timer_init(loop_, &delayed->timer); // Timers may not guarantee queue ordering of events with the same delay if // 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)); - scheduled_delayed_tasks_.push_back(delayed); + scheduled_delayed_tasks_.push_back(delayed.release()); } - while (Task* task = foreground_tasks_.Pop()) { + while (std::unique_ptr task = foreground_tasks_.Pop()) { did_work = true; - RunForegroundTask(task); + RunForegroundTask(std::move(task)); } return did_work; } void NodePlatform::CallOnBackgroundThread(Task* task, ExpectedRuntime expected_runtime) { - background_tasks_.Push(task); + background_tasks_.Push(std::unique_ptr(task)); } PerIsolatePlatformData* NodePlatform::ForIsolate(Isolate* isolate) { @@ -205,14 +205,14 @@ PerIsolatePlatformData* NodePlatform::ForIsolate(Isolate* isolate) { } void NodePlatform::CallOnForegroundThread(Isolate* isolate, Task* task) { - ForIsolate(isolate)->CallOnForegroundThread(task); + ForIsolate(isolate)->CallOnForegroundThread(std::unique_ptr(task)); } void NodePlatform::CallDelayedOnForegroundThread(Isolate* isolate, Task* task, double delay_in_seconds) { - ForIsolate(isolate)->CallDelayedOnForegroundThread(task, - delay_in_seconds); + ForIsolate(isolate)->CallDelayedOnForegroundThread( + std::unique_ptr(task), delay_in_seconds); } void NodePlatform::FlushForegroundTasks(v8::Isolate* isolate) { @@ -240,34 +240,34 @@ TaskQueue::TaskQueue() outstanding_tasks_(0), stopped_(false), task_queue_() { } template -void TaskQueue::Push(T* task) { +void TaskQueue::Push(std::unique_ptr task) { Mutex::ScopedLock scoped_lock(lock_); outstanding_tasks_++; - task_queue_.push(task); + task_queue_.push(std::move(task)); tasks_available_.Signal(scoped_lock); } template -T* TaskQueue::Pop() { +std::unique_ptr TaskQueue::Pop() { Mutex::ScopedLock scoped_lock(lock_); - T* result = nullptr; - if (!task_queue_.empty()) { - result = task_queue_.front(); - task_queue_.pop(); + if (task_queue_.empty()) { + return std::unique_ptr(nullptr); } + std::unique_ptr result = std::move(task_queue_.front()); + task_queue_.pop(); return result; } template -T* TaskQueue::BlockingPop() { +std::unique_ptr TaskQueue::BlockingPop() { Mutex::ScopedLock scoped_lock(lock_); while (task_queue_.empty() && !stopped_) { tasks_available_.Wait(scoped_lock); } if (stopped_) { - return nullptr; + return std::unique_ptr(nullptr); } - T* result = task_queue_.front(); + std::unique_ptr result = std::move(task_queue_.front()); task_queue_.pop(); return result; } diff --git a/src/node_platform.h b/src/node_platform.h index 73c2509e1a0052..584506ae1f114a 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -22,9 +22,9 @@ class TaskQueue { TaskQueue(); ~TaskQueue() {} - void Push(T* task); - T* Pop(); - T* BlockingPop(); + void Push(std::unique_ptr task); + std::unique_ptr Pop(); + std::unique_ptr BlockingPop(); void NotifyOfCompletion(); void BlockingDrain(); void Stop(); @@ -35,11 +35,11 @@ class TaskQueue { ConditionVariable tasks_drained_; int outstanding_tasks_; bool stopped_; - std::queue task_queue_; + std::queue> task_queue_; }; struct DelayedTask { - v8::Task* task; + std::unique_ptr task; uv_timer_t timer; double timeout; PerIsolatePlatformData* platform_data; @@ -50,8 +50,9 @@ class PerIsolatePlatformData { PerIsolatePlatformData(v8::Isolate* isolate, uv_loop_t* loop); ~PerIsolatePlatformData(); - void CallOnForegroundThread(v8::Task* task); - void CallDelayedOnForegroundThread(v8::Task* task, double delay_in_seconds); + void CallOnForegroundThread(std::unique_ptr task); + void CallDelayedOnForegroundThread(std::unique_ptr task, + double delay_in_seconds); void Shutdown(); @@ -64,7 +65,7 @@ class PerIsolatePlatformData { private: static void FlushTasks(uv_async_t* handle); - static void RunForegroundTask(v8::Task* task); + static void RunForegroundTask(std::unique_ptr task); static void RunForegroundTask(uv_timer_t* timer); int ref_count_ = 1;