Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Deno.core.setGCObserver() for callbacks invoked on GC #2106

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -132,5 +132,5 @@ jobs:
- third_party/depot_tools/gn gen target/debug
- export ASAN_OPTIONS=detect_leaks=1
- ./tools/build.py test_cc
- ./target/debug/test_cc
- ./target/debug/test_cc --expose-gc

5 changes: 5 additions & 0 deletions core/libdeno/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ Deno* deno_new(deno_config config) {
deno::InitializeContext(isolate, context);
}
d->context_.Reset(isolate, context);

d->gc_observer_private_symbol_.Reset(
isolate,
v8::Private::New(isolate, v8::String::NewFromUtf8(
isolate, "deno:gc_observer:symbol")));
}

return reinterpret_cast<Deno*>(d);
Expand Down
69 changes: 69 additions & 0 deletions core/libdeno/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,68 @@ void ErrorToJSON(const v8::FunctionCallbackInfo<v8::Value>& args) {
args.GetReturnValue().Set(v8_str(json_string.c_str()));
}

class GCObserver {
Copy link
Member

@ry ry Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm embarrassed to say but I totally forgot that V8 has this weak callback functionality.

I don't like introducing new nomenclature like "GCObserver". Maybe call it"PersistentWithWeakCallback" ?

Have you looked at the TracedGlobal class? It seems like that could be used here as well?

Copy link
Contributor Author

@kevinkassimo kevinkassimo Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems requires manual setup of a custom EmbedderHeapTracer implementation, and closer to kInternalFields (have some examples here).

Before experimenting with this I actually want to set if the observed is actually GC-ed after a proper use of callback instead of being resurrected and leaked. (Theoretically it should not, but this still needs to be examined before we can use this with confidence) (Though I'm having trouble now with native syntaxes in snapshots to use the %DebugTrackRetainingPath builtin. Also attempts to run --prof on Deno failed due to a flag parsing issue)

private:
v8::Persistent<v8::Object> wrapper_;
v8::Persistent<v8::Function> callback_;
v8::Persistent<v8::Context> context_;

public:
void Register(v8::Isolate* isolate, v8::Local<v8::Object> obj) {
wrapper_.Reset(isolate, obj);
wrapper_.SetWeak(this, WeakCallback,
// Before the object is actually gc-ed.
v8::WeakCallbackType::kFinalizer);
}

void SetCallback(v8::Isolate* isolate, v8::Local<v8::Function> callback) {
context_.Reset(isolate, isolate->GetCurrentContext());
callback_.Reset(isolate, callback);
}

static void WeakCallback(const v8::WeakCallbackInfo<GCObserver>& data) {
auto* self = data.GetParameter();
auto* isolate = v8::Isolate::GetCurrent();
v8::Locker locker(isolate);
v8::HandleScope handle_scope(isolate);
auto context = self->context_.Get(isolate);
v8::Local<v8::Value> args[1];
args[0] = self->wrapper_.Get(isolate);
// Invoke callback.
(void)self->callback_.Get(isolate)->Call(context, context->Global(), 1,
args);
// Reset handles.
self->wrapper_.Reset();
self->callback_.Reset();
self->context_.Reset();

delete self;
}
};

void SetGCObserver(const v8::FunctionCallbackInfo<v8::Value>& args) {
CHECK_EQ(args.Length(), 2);
auto* isolate = args.GetIsolate();
v8::Locker locker(isolate);
v8::Isolate::Scope isolate_scope(isolate);
v8::EscapableHandleScope handle_scope(isolate);

DenoIsolate* d = DenoIsolate::FromIsolate(isolate);
auto context = d->context_.Get(isolate);
auto obj = args[0].As<v8::Object>();
auto callback = args[1].As<v8::Function>();

auto observer = new GCObserver();
observer->Register(isolate, obj);
observer->SetCallback(isolate, callback);
// Make LSAN happy
obj->SetPrivate(context, d->gc_observer_private_symbol_.Get(isolate),
v8::External::New(isolate, observer));

handle_scope.Escape(obj);
args.GetReturnValue().Set(obj);
}

v8::Local<v8::Uint8Array> ImportBuf(DenoIsolate* d, deno_buf buf) {
// Do not use ImportBuf with zero_copy buffers.
DCHECK_EQ(buf.zero_copy_id, 0);
Expand Down Expand Up @@ -514,6 +576,13 @@ void InitializeContext(v8::Isolate* isolate, v8::Local<v8::Context> context) {
CHECK(core_val->Set(context, deno::v8_str("errorToJSON"), error_to_json_val)
.FromJust());

auto set_gc_observer_tmpl = v8::FunctionTemplate::New(isolate, SetGCObserver);
auto set_gc_observer_val =
set_gc_observer_tmpl->GetFunction(context).ToLocalChecked();
CHECK(
core_val->Set(context, deno::v8_str("setGCObserver"), set_gc_observer_val)
.FromJust());

CHECK(core_val->SetAccessor(context, deno::v8_str("shared"), Shared)
.FromJust());
}
Expand Down
3 changes: 3 additions & 0 deletions core/libdeno/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class DenoIsolate {
deno_resolve_cb resolve_cb_;

v8::Persistent<v8::Context> context_;
v8::Persistent<v8::Private> gc_observer_private_symbol_;
std::map<size_t, v8::Persistent<v8::Value>> zero_copy_map_;
std::map<int, v8::Persistent<v8::Value>> pending_promise_map_;
std::string last_exception_;
Expand Down Expand Up @@ -155,6 +156,7 @@ void Recv(const v8::FunctionCallbackInfo<v8::Value>& args);
void Send(const v8::FunctionCallbackInfo<v8::Value>& args);
void EvalContext(const v8::FunctionCallbackInfo<v8::Value>& args);
void ErrorToJSON(const v8::FunctionCallbackInfo<v8::Value>& args);
void SetGCObserver(const v8::FunctionCallbackInfo<v8::Value>& args);
void Shared(v8::Local<v8::Name> property,
const v8::PropertyCallbackInfo<v8::Value>& info);
void MessageCallback(v8::Local<v8::Message> message, v8::Local<v8::Value> data);
Expand All @@ -164,6 +166,7 @@ static intptr_t external_references[] = {
reinterpret_cast<intptr_t>(Send),
reinterpret_cast<intptr_t>(EvalContext),
reinterpret_cast<intptr_t>(ErrorToJSON),
reinterpret_cast<intptr_t>(SetGCObserver),
reinterpret_cast<intptr_t>(Shared),
reinterpret_cast<intptr_t>(MessageCallback),
0};
Expand Down
3 changes: 3 additions & 0 deletions core/libdeno/libdeno.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,7 @@ declare interface DenoCore {
evalContext(code: string): [any, EvalErrorInfo | null];

errorToJSON: (e: Error) => string;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
setGCObserver: <T>(o: T, callback: (o: T) => void) => T;
Copy link
Member

@ry ry Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better named "setWeakCallback"

I also think this would be a good place for some JSDOC. The docs from v8.h should be forwarded here:

/** Install a finalization callback on this object.
 *  NOTE: There is no guarantee as to *when* or even *if* the callback is
 *  invoked. The invocation is performed solely on a best effort basis.
 *  As always, GC-based finalization should *not* be relied upon for any
 *  critical form of resource management!
 */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really so bad?
Is it guaranteed to execute prior to the program exiting?
What then should / can this be used for?

Copy link
Contributor Author

@kevinkassimo kevinkassimo Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hayd There is no guarantee about when this will be invoked. It is possible that we can force a GC on program exit, for sanity (just like how fds are closed on program exit) (the callback should always be invoked if the object is actually being GCed)

That being said, this is way better than simply leaving program hang forever due to a resource not closed while losing all reference to it. Also I do remember v8 now has some very interesting GC policies.

Node also uses weak callbacks for cleaning up resources e.g. for the vm module

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does explicit gc ensure it calls? Ry's doc/comment suggests there's an "if" here too.

I like the idea of an explicit gc call prior to exit.

}
7 changes: 7 additions & 0 deletions core/libdeno/libdeno_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,13 @@ TEST(LibDenoTest, LibDenoEvalContextError) {
deno_delete(d);
}

TEST(LibDenoTest, LibDenoGCObserver) {
Deno* d = deno_new(deno_config{0, snapshot, empty, nullptr});
deno_execute(d, nullptr, "a.js", "LibDenoGCObserver();");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/LibDenoGCObserver/weakCallback/

EXPECT_EQ(nullptr, deno_last_exception(d));
deno_delete(d);
}

TEST(LibDenoTest, SharedAtomics) {
int32_t s[] = {0, 1, 2};
deno_buf shared = {nullptr, 0, reinterpret_cast<uint8_t*>(s), sizeof s, 0};
Expand Down
62 changes: 62 additions & 0 deletions core/libdeno/libdeno_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,3 +195,65 @@ global.LibDenoEvalContextError = () => {
assert(!errInfo5.isCompileError); // is NOT a compilation error! (just eval)
assert(errInfo5.thrown.message === "Unexpected end of input");
};

global.LibDenoGCObserver = () => {
// Basic usage.
let num = 1;
Copy link
Member

@ry ry Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/num/counter/

const incr = () => {
Deno.core.print("Destroy callback invoked\n");
num = num + 1;
};
// IIFE. Simply using scope might not work.
(() => {
const o = Deno.core.setGCObserver({}, incr);
assert(!!o);
})();
// Guarantee GC.
gc();
assert(num === 2);

// Accessing the gc-ed object in callback.
let num2 = 0;
// IIFE. Simply using scope might not work.
(() => {
const obj = { a: 10 };
const incr2 = o => {
Deno.core.print("Destroy callback invoked 2\n");
num2 = o.a;
};
Deno.core.setGCObserver(obj, incr2);
})();
// Guarantee GC.
gc();
assert(num2 === 10);
Copy link
Member

@ry ry Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stanza, using num2, seems like it could be a standalone test: weakCallback2. It would be good to break up the test into distinct components so that they will fail individually during test_cc


// Simulating a connection goes out of scope
// for auto closing.
let isClosed = false;
let ID = 12345;
const fakeClose = id => {
if (id === ID) {
isClosed = true;
}
};
class FakeConn {
constructor(rid) {
this.rid = rid;
}
close() {
Deno.core.print("Closing fake connection\n");
fakeClose(this.rid);
}
}
// IIFE. Simply using scope might not work.
(() => {
const conn = new FakeConn(12345);
Deno.core.setGCObserver(conn, c => {
Deno.core.print("Destroy callback invoked 3\n");
c.close();
});
})();
// Guarantee GC.
gc();
assert(isClosed);
};
2 changes: 1 addition & 1 deletion tools/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def main(argv):

test_cc = os.path.join(build_dir, "test_cc" + executable_suffix)
check_exists(test_cc)
run([test_cc])
run([test_cc, "--expose-gc"])
Copy link
Member

@ry ry Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the --expose-gc into C++ so that it is always specified. That should be done here:

deno_set_v8_flags(&argc, argv);

(It's slightly annoying, I guess you have to allocate a new argv with size argc+1 and copy over the existing flags, and add the new "--expose-gc")


test_rs = os.path.join(build_dir, "test_rs" + executable_suffix)
check_exists(test_rs)
Expand Down