Skip to content

Commit

Permalink
src: use uv_async_t for WeakRefs
Browse files Browse the repository at this point in the history
Schedule a task on the main event loop, similar to what the HTML
spec recommends for browsers.

Alternative to #30198

PR-URL: #30616
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
addaleax committed Nov 29, 2019
1 parent ff48009 commit fad952a
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 6 deletions.
1 change: 1 addition & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,7 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) {
inline void Environment::RegisterFinalizationGroupForCleanup(
v8::Local<v8::FinalizationGroup> group) {
cleanup_finalization_groups_.emplace_back(isolate(), group);
uv_async_send(&cleanup_finalization_groups_async_);
}

size_t CleanupHookCallback::Hash::operator()(
Expand Down
31 changes: 26 additions & 5 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,17 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
// will be recorded with state=IDLE.
uv_prepare_init(event_loop(), &idle_prepare_handle_);
uv_check_init(event_loop(), &idle_check_handle_);
uv_async_init(
event_loop(),
&cleanup_finalization_groups_async_,
[](uv_async_t* async) {
Environment* env = ContainerOf(
&Environment::cleanup_finalization_groups_async_, async);
env->CleanupFinalizationGroups();
});
uv_unref(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
uv_unref(reinterpret_cast<uv_handle_t*>(&idle_check_handle_));
uv_unref(reinterpret_cast<uv_handle_t*>(&cleanup_finalization_groups_async_));

thread_stopper()->Install(
this, static_cast<void*>(this), [](uv_async_t* handle) {
Expand Down Expand Up @@ -523,6 +532,10 @@ void Environment::RegisterHandleCleanups() {
reinterpret_cast<uv_handle_t*>(&idle_check_handle_),
close_and_finish,
nullptr);
RegisterHandleCleanup(
reinterpret_cast<uv_handle_t*>(&cleanup_finalization_groups_async_),
close_and_finish,
nullptr);
}

void Environment::CleanupHandles() {
Expand Down Expand Up @@ -1046,19 +1059,27 @@ char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
return new_data;
}

bool Environment::RunWeakRefCleanup() {
void Environment::RunWeakRefCleanup() {
isolate()->ClearKeptObjects();
}

while (!cleanup_finalization_groups_.empty()) {
void Environment::CleanupFinalizationGroups() {
HandleScope handle_scope(isolate());
Context::Scope context_scope(context());
TryCatchScope try_catch(this);

while (!cleanup_finalization_groups_.empty() && can_call_into_js()) {
Local<FinalizationGroup> fg =
cleanup_finalization_groups_.front().Get(isolate());
cleanup_finalization_groups_.pop_front();
if (!FinalizationGroup::Cleanup(fg).FromMaybe(false)) {
return false;
if (try_catch.HasCaught() && !try_catch.HasTerminated())
errors::TriggerUncaughtException(isolate(), try_catch);
// Re-schedule the execution of the remainder of the queue.
uv_async_send(&cleanup_finalization_groups_async_);
return;
}
}

return true;
}

void AsyncRequest::Install(Environment* env, void* data, uv_async_cb target) {
Expand Down
4 changes: 3 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,8 @@ class Environment : public MemoryRetainer {
void RunAtExitCallbacks();

void RegisterFinalizationGroupForCleanup(v8::Local<v8::FinalizationGroup> fg);
bool RunWeakRefCleanup();
void RunWeakRefCleanup();
void CleanupFinalizationGroups();

// Strings and private symbols are shared across shared contexts
// The getters simply proxy to the per-isolate primitive.
Expand Down Expand Up @@ -1270,6 +1271,7 @@ class Environment : public MemoryRetainer {
uv_idle_t immediate_idle_handle_;
uv_prepare_t idle_prepare_handle_;
uv_check_t idle_check_handle_;
uv_async_t cleanup_finalization_groups_async_;
bool profiler_idle_notifier_started_ = false;

AsyncHooks async_hooks_;
Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-finalization-group-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ setTimeout(() => {
name: 'Error',
message: 'test',
});

// Give the callbacks scheduled by global.gc() time to run, as the underlying
// uv_async_t is unref’ed.
setTimeout(() => {}, 200);
}, 200);

process.on('uncaughtException', common.mustCall());
25 changes: 25 additions & 0 deletions test/parallel/test-finalization-group-regular-gc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Flags: --harmony-weak-refs
'use strict';
require('../common');
const assert = require('assert');

// Test that finalization callbacks do not crash when caused through a regular
// GC (not global.gc()).

const start = Date.now();
const g = new globalThis.FinalizationGroup(() => {
const diff = Date.now() - start;
assert(diff < 10000, `${diff} >= 10000`);
});
g.register({}, 42);

setImmediate(() => {
const arr = [];
// Build up enough memory usage to hopefully trigger a platform task but not
// enough to trigger GC as an interrupt.
while (arr.length < 1000000) arr.push([]);

setTimeout(() => {
g; // Keep reference alive.
}, 200000).unref();
});

0 comments on commit fad952a

Please sign in to comment.