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

Open
wants to merge 2 commits into
base: master
from

Conversation

4 participants
@kevinkassimo
Copy link
Contributor

commented Apr 12, 2019

Deno.core.setGCObserver(target, handler) sets handler: (target) => void to be invoked on target immediately before being GC-ed e.g. to clear up open resources such as connections.

Sample usage (as shown in libdeno tests):

  // 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);

@kevinkassimo kevinkassimo force-pushed the kevinkassimo:core/gc_observer branch from 2fe56f9 to 395471c Apr 12, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

Does this have the same name in d8 and node?

I totally didn’t know about this feature!

@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@ry Er this is just a custom wrapper using WeakCallback... Not very familiar with Node's codebase but I would believe it to be similar to ObjectWrap in a way, but instead of just calling C++ cleanup code on WeakCallback we also try invoke attached JS function. The v8::WeakCallbackType::kFinalizer ensures that the callback would be invoked before object got GC-ed instead of after, which makes calling JS code feasible here...

(Seems cannot really find any usage of kFinalizer in Node's codebase...)

@hayd

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Is IIFE required and not scope just for tests/the gc call or something fundamental?
What happens if you do something silly inside the callback like push c back into an array/somewhere live?

Perhaps this could be called onDrop 😬

@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@hayd From my experiment, only by going out of function scope is garbage collector guaranteed to dispose the unreferenced object.

Also from v8 header:

kFinalizer will pass a void* parameter back, but is invoked before the object is actually collected, so it can be resurrected. In the last case, it is not possible to request a second pass callback.

Basically if user really decides to do the "silly" stuff in the callback, the object would not get GC-ed, and the observer would no longer have any effect on this object anymore: we have called Reset on the persistent handles in this structure in the initial weak callback attempt, so this weak callback would no longer be affiliated with the object.

I agree setGCObserver sounds too funny. Probably something like addDropListener? onDrop sounds okay too.

@@ -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;

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Collaborator

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!
 */

This comment has been minimized.

Copy link
@hayd

hayd Apr 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Apr 13, 2019

Author Contributor

@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

This comment has been minimized.

Copy link
@hayd

hayd Apr 13, 2019

Contributor

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.

@@ -162,6 +162,68 @@ void ErrorToJSON(const v8::FunctionCallbackInfo<v8::Value>& args) {
args.GetReturnValue().Set(v8_str(json_string.c_str()));
}

class GCObserver {

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Apr 13, 2019

Author Contributor

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)


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

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Collaborator

s/num/counter/

})();
// Guarantee GC.
gc();
assert(num2 === 10);

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Collaborator

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

@@ -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();");

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Collaborator

s/LibDenoGCObserver/weakCallback/

@@ -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"])

This comment has been minimized.

Copy link
@ry

ry Apr 13, 2019

Collaborator

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")

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

Deno.core.setGCObserver(target, handler) sets handler: (target) => void to be invoked on target immediately before being GC-ed e.g. to clear up open resources such as connections.

This is generally speaking not a good idea, except maybe for debugging purposes.
The reason is that the GC is "lazy"; it only runs when necessary because of memory pressure.
Other types of resources are not considered in the decision making on whether/when the gc runs, so you can't rely on it to do any good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.