Skip to content

Commit

Permalink
src: make implementing CancelPendingDelayedTasks for platform optional
Browse files Browse the repository at this point in the history
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: #30034
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
  • Loading branch information
addaleax authored and MylesBorins committed Oct 23, 2019
1 parent 8fcc039 commit ed80c23
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 31 deletions.
9 changes: 7 additions & 2 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
1 change: 0 additions & 1 deletion src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
44 changes: 26 additions & 18 deletions src/node_platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<uv_handle_t*>(flush_tasks_),
[](uv_handle_t* handle) {
std::unique_ptr<ShutdownCbList> callbacks(
static_cast<ShutdownCbList*>(handle->data));
for (const auto& callback : *callbacks)
callback.cb(callback.data);
delete reinterpret_cast<uv_async_t*>(handle);
std::unique_ptr<uv_async_t> flush_tasks {
reinterpret_cast<uv_async_t*>(handle) };
PerIsolatePlatformData* platform_data =
static_cast<PerIsolatePlatformData*>(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) {
Expand Down Expand Up @@ -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<PerIsolatePlatformData> per_isolate = ForIsolate(isolate);

Expand All @@ -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<uv_handle_t*>(&delayed->timer));
uv_handle_count_++;

scheduled_delayed_tasks_.emplace_back(delayed.release(),
[](DelayedTask* delayed) {
uv_close(reinterpret_cast<uv_handle_t*>(&delayed->timer),
[](uv_handle_t* handle) {
delete static_cast<DelayedTask*>(handle->data);
std::unique_ptr<DelayedTask> task {
static_cast<DelayedTask*>(handle->data) };
task->platform_data->DecreaseHandleCount();
});
});
}
Expand Down Expand Up @@ -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<v8::TaskRunner>
Expand Down Expand Up @@ -548,4 +554,6 @@ std::queue<std::unique_ptr<T>> TaskQueue<T>::PopAll() {
return result;
}

void MultiIsolatePlatform::CancelPendingDelayedTasks(Isolate* isolate) {}

} // namespace node
8 changes: 5 additions & 3 deletions src/node_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Task> task);
Expand All @@ -95,14 +95,17 @@ class PerIsolatePlatformData :
};
typedef std::vector<ShutdownCallback> ShutdownCbList;
ShutdownCbList shutdown_callbacks_;
// shared_ptr to self to keep this object alive during shutdown.
std::shared_ptr<PerIsolatePlatformData> self_reference_;
uint32_t uv_handle_count_ = 1; // 1 = flush_tasks_

uv_loop_t* const loop_;
uv_async_t* flush_tasks_ = nullptr;
TaskQueue<v8::Task> foreground_tasks_;
TaskQueue<DelayedTask> foreground_delayed_tasks_;

// Use a custom deleter because libuv needs to close the handle first.
typedef std::unique_ptr<DelayedTask, std::function<void(DelayedTask*)>>
typedef std::unique_ptr<DelayedTask, void(*)(DelayedTask*)>
DelayedTaskPointer;
std::vector<DelayedTaskPointer> scheduled_delayed_tasks_;
};
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 0 additions & 5 deletions src/node_v8_platform-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 0 additions & 2 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,6 @@ class WorkerThreadData {
w_->isolate_ = nullptr;
}

w_->platform_->CancelPendingDelayedTasks(isolate);

bool platform_finished = false;

isolate_data_.reset();
Expand Down

0 comments on commit ed80c23

Please sign in to comment.