Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit abfac96

Browse filesBrowse files
addaleaxtargos
addaleax
authored and
targos
committed
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: #30034 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Shelley Vohr <codebytere@gmail.com>
1 parent 693bf73 commit abfac96
Copy full SHA for abfac96

File tree

Expand file treeCollapse file tree

6 files changed

+38
-31
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+38
-31
lines changed
Open diff view settings
Collapse file

‎src/node.h‎

Copy file name to clipboardExpand all lines: src/node.h
+7-2Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,11 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
254254
// flushing.
255255
virtual bool FlushForegroundTasks(v8::Isolate* isolate) = 0;
256256
virtual void DrainTasks(v8::Isolate* isolate) = 0;
257-
virtual void CancelPendingDelayedTasks(v8::Isolate* isolate) = 0;
257+
258+
// TODO(addaleax): Remove this, it is unnecessary.
259+
// This would currently be called before `UnregisterIsolate()` but will be
260+
// folded into it in the future.
261+
virtual void CancelPendingDelayedTasks(v8::Isolate* isolate);
258262

259263
// This needs to be called between the calls to `Isolate::Allocate()` and
260264
// `Isolate::Initialize()`, so that initialization can already start
@@ -264,7 +268,8 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
264268
virtual void RegisterIsolate(v8::Isolate* isolate,
265269
struct uv_loop_s* loop) = 0;
266270
// This needs to be called right before calling `Isolate::Dispose()`.
267-
// This function may only be called once per `Isolate`.
271+
// This function may only be called once per `Isolate`, and discard any
272+
// pending delayed tasks scheduled for that isolate.
268273
virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
269274
// The platform should call the passed function once all state associated
270275
// with the given isolate has been cleaned up. This can, but does not have to,
Collapse file

‎src/node_main_instance.cc‎

Copy file name to clipboardExpand all lines: src/node_main_instance.cc
-1Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,6 @@ int NodeMainInstance::Run() {
155155
RunAtExit(env.get());
156156

157157
per_process::v8_platform.DrainVMTasks(isolate_);
158-
per_process::v8_platform.CancelVMTasks(isolate_);
159158

160159
#if defined(LEAK_SANITIZER)
161160
__lsan_do_leak_check();
Collapse file

‎src/node_platform.cc‎

Copy file name to clipboardExpand all lines: src/node_platform.cc
+26-18Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -285,22 +285,33 @@ void PerIsolatePlatformData::Shutdown() {
285285
// effectively deleting the tasks instead of running them.
286286
foreground_delayed_tasks_.PopAll();
287287
foreground_tasks_.PopAll();
288+
scheduled_delayed_tasks_.clear();
288289

289-
CancelPendingDelayedTasks();
290-
291-
ShutdownCbList* copy = new ShutdownCbList(std::move(shutdown_callbacks_));
292-
flush_tasks_->data = copy;
290+
// Both destroying the scheduled_delayed_tasks_ lists and closing
291+
// flush_tasks_ handle add tasks to the event loop. We keep a count of all
292+
// non-closed handles, and when that reaches zero, we inform any shutdown
293+
// callbacks that the platform is done as far as this Isolate is concerned.
294+
self_reference_ = shared_from_this();
293295
uv_close(reinterpret_cast<uv_handle_t*>(flush_tasks_),
294296
[](uv_handle_t* handle) {
295-
std::unique_ptr<ShutdownCbList> callbacks(
296-
static_cast<ShutdownCbList*>(handle->data));
297-
for (const auto& callback : *callbacks)
298-
callback.cb(callback.data);
299-
delete reinterpret_cast<uv_async_t*>(handle);
297+
std::unique_ptr<uv_async_t> flush_tasks {
298+
reinterpret_cast<uv_async_t*>(handle) };
299+
PerIsolatePlatformData* platform_data =
300+
static_cast<PerIsolatePlatformData*>(flush_tasks->data);
301+
platform_data->DecreaseHandleCount();
302+
platform_data->self_reference_.reset();
300303
});
301304
flush_tasks_ = nullptr;
302305
}
303306

307+
void PerIsolatePlatformData::DecreaseHandleCount() {
308+
CHECK_GE(uv_handle_count_, 1);
309+
if (--uv_handle_count_ == 0) {
310+
for (const auto& callback : shutdown_callbacks_)
311+
callback.cb(callback.data);
312+
}
313+
}
314+
304315
NodePlatform::NodePlatform(int thread_pool_size,
305316
TracingController* tracing_controller) {
306317
if (tracing_controller) {
@@ -382,10 +393,6 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) {
382393
delayed->platform_data->DeleteFromScheduledTasks(delayed);
383394
}
384395

385-
void PerIsolatePlatformData::CancelPendingDelayedTasks() {
386-
scheduled_delayed_tasks_.clear();
387-
}
388-
389396
void NodePlatform::DrainTasks(Isolate* isolate) {
390397
std::shared_ptr<PerIsolatePlatformData> per_isolate = ForIsolate(isolate);
391398

@@ -409,12 +416,15 @@ bool PerIsolatePlatformData::FlushForegroundTasksInternal() {
409416
// the delay is non-zero. This should not be a problem in practice.
410417
uv_timer_start(&delayed->timer, RunForegroundTask, delay_millis, 0);
411418
uv_unref(reinterpret_cast<uv_handle_t*>(&delayed->timer));
419+
uv_handle_count_++;
412420

413421
scheduled_delayed_tasks_.emplace_back(delayed.release(),
414422
[](DelayedTask* delayed) {
415423
uv_close(reinterpret_cast<uv_handle_t*>(&delayed->timer),
416424
[](uv_handle_t* handle) {
417-
delete static_cast<DelayedTask*>(handle->data);
425+
std::unique_ptr<DelayedTask> task {
426+
static_cast<DelayedTask*>(handle->data) };
427+
task->platform_data->DecreaseHandleCount();
418428
});
419429
});
420430
}
@@ -454,10 +464,6 @@ bool NodePlatform::FlushForegroundTasks(Isolate* isolate) {
454464
return ForIsolate(isolate)->FlushForegroundTasksInternal();
455465
}
456466

457-
void NodePlatform::CancelPendingDelayedTasks(Isolate* isolate) {
458-
ForIsolate(isolate)->CancelPendingDelayedTasks();
459-
}
460-
461467
bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; }
462468

463469
std::shared_ptr<v8::TaskRunner>
@@ -548,4 +554,6 @@ std::queue<std::unique_ptr<T>> TaskQueue<T>::PopAll() {
548554
return result;
549555
}
550556

557+
void MultiIsolatePlatform::CancelPendingDelayedTasks(Isolate* isolate) {}
558+
551559
} // namespace node
Collapse file

‎src/node_platform.h‎

Copy file name to clipboardExpand all lines: src/node_platform.h
+5-3Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,12 @@ class PerIsolatePlatformData :
7878
// posted during flushing of the queue are postponed until the next
7979
// flushing.
8080
bool FlushForegroundTasksInternal();
81-
void CancelPendingDelayedTasks();
8281

8382
const uv_loop_t* event_loop() const { return loop_; }
8483

8584
private:
8685
void DeleteFromScheduledTasks(DelayedTask* task);
86+
void DecreaseHandleCount();
8787

8888
static void FlushTasks(uv_async_t* handle);
8989
static void RunForegroundTask(std::unique_ptr<v8::Task> task);
@@ -95,14 +95,17 @@ class PerIsolatePlatformData :
9595
};
9696
typedef std::vector<ShutdownCallback> ShutdownCbList;
9797
ShutdownCbList shutdown_callbacks_;
98+
// shared_ptr to self to keep this object alive during shutdown.
99+
std::shared_ptr<PerIsolatePlatformData> self_reference_;
100+
uint32_t uv_handle_count_ = 1; // 1 = flush_tasks_
98101

99102
uv_loop_t* const loop_;
100103
uv_async_t* flush_tasks_ = nullptr;
101104
TaskQueue<v8::Task> foreground_tasks_;
102105
TaskQueue<DelayedTask> foreground_delayed_tasks_;
103106

104107
// Use a custom deleter because libuv needs to close the handle first.
105-
typedef std::unique_ptr<DelayedTask, std::function<void(DelayedTask*)>>
108+
typedef std::unique_ptr<DelayedTask, void(*)(DelayedTask*)>
106109
DelayedTaskPointer;
107110
std::vector<DelayedTaskPointer> scheduled_delayed_tasks_;
108111
};
@@ -137,7 +140,6 @@ class NodePlatform : public MultiIsolatePlatform {
137140
~NodePlatform() override = default;
138141

139142
void DrainTasks(v8::Isolate* isolate) override;
140-
void CancelPendingDelayedTasks(v8::Isolate* isolate) override;
141143
void Shutdown();
142144

143145
// v8::Platform implementation.
Collapse file

‎src/node_v8_platform-inl.h‎

Copy file name to clipboardExpand all lines: src/node_v8_platform-inl.h
-5Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -113,10 +113,6 @@ struct V8Platform {
113113
platform_->DrainTasks(isolate);
114114
}
115115

116-
inline void CancelVMTasks(v8::Isolate* isolate) {
117-
platform_->CancelPendingDelayedTasks(isolate);
118-
}
119-
120116
inline void StartTracingAgent() {
121117
// Attach a new NodeTraceWriter only if this function hasn't been called
122118
// before.
@@ -150,7 +146,6 @@ struct V8Platform {
150146
inline void Initialize(int thread_pool_size) {}
151147
inline void Dispose() {}
152148
inline void DrainVMTasks(v8::Isolate* isolate) {}
153-
inline void CancelVMTasks(v8::Isolate* isolate) {}
154149
inline void StartTracingAgent() {
155150
if (!per_process::cli_options->trace_event_categories.empty()) {
156151
fprintf(stderr,
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,6 @@ class WorkerThreadData {
148148
w_->isolate_ = nullptr;
149149
}
150150

151-
w_->platform_->CancelPendingDelayedTasks(isolate);
152-
153151
bool platform_finished = false;
154152

155153
isolate_data_.reset();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.