Node: async_hooks: no way to EmitDestroy on garbage collection with the high-level Embedder API

[akdor1154]

• Version: 8.7
• Platform:
• Subsystem: async_hooks

Let's say I make a super simple AsyncResource, that lets me bind a function to always run in a particular executionId.

class BoundFunction extends asyncHooks.AsyncResource {

  constructor(f, bindTimeAsyncId) {
    super('CONTEXT_BIND', bindTimeAsyncId);
    this.f = f;
  }

  run() {
    this.emitBefore();
    const ret = this.f();
    this.emitAfter();
    return ret;
  }
}

I can use it with the following wrapper:

function bind(f) {

  const bindTimeAsyncId = asyncHooks.executionAsyncId();
  const boundF = new BoundHook(f, bindTimeAsyncId);

  return () => boundF.run();
}

However, it seems I have no way to schedule cleanup. Let's say I do

const f = () => console.log(`hello from ${asyncHooks.executionAsyncId()}`);
const boundF = bind(f);

I want BoundFunction#emitDestroy() to run once the GC grabs boundF. The docs seem to say that this is a normal approach to async resources, by

Note: Some resources depend on garbage collection for cleanup, so if a reference is made to the resource object passed to init it is possible that destroy will never be called, causing a memory leak in the application. If the resource does not depend on garbage collection, then this will not be an issue.

However, as far as I can see the JS Embedder API gives me no way to achieve this behaviour.

[addaleax]

+1 – the only thing we need to be careful about is that the API we expose here makes garbage collection of arbitrary objects visible. I guess there’s no way around that, though? Also, should AsyncResource do this by default?

/cc @nodejs/async_hooks

[akdor1154]

As a non-expert on the innards of this, it would make sense if AsyncResource did it by default. That way GC remains hidden. Not sure if there should be an opt out method or not. (just check if it's already been emitted in the default implementation?)

[AndreasMadsen]

Also, should AsyncResource do this by default?

I don't see why we would make it optional. Either the users hold a reference because they want to call emitDestroy() themselves. Then it can't be GC'ed anyway. Or they don't hold a reference and they intend for it to happen automatically.

[addaleax]

@AndreasMadsen The thing is, if emitDestroy() is called manually, do we keep track of that? Because at some point everything is going to get GC’ed, and we don’t want 2 destroy calls, right?

[AndreasMadsen]

We also need to be aware of the emitInit Sensitive API. In that case, we could attach it to the handle object, but the user will hold a reference to newUid not the handle object. And they may very well want to run:

emitInit(5, 'TYPE', 10, { sql: 'SELECT an FROM examples ' })
emitBefore(5, 10)
emitAfter(5)
emitDestroy(5)

The thing is, if emitDestroy() is called manually, do we keep track of that?

Right. In the AsyncResource we can easily keep track of that. I think we can just remove the on('gc') listener when emitDestroy is called. We are making a call to C++ anyway, so it should be a big deal. In the Sensitive API case it much more complicated :/ – Yet another reason I want it removed :p

[addaleax]

I’m labeling this good first issue because it isn’t done yet, it really should happen, I might not have the time do it but would definitely be available for any questions/guidance in getting it done (same handle on freenode/twitter (open dms), if that matters to anybody). It’s requires a bit of work, but I think it would make a great first contribution.

[Sebmaster]

I've got an initial draft of this, will try to open a PR either tomorrow or Monday at the latest.

[AndreasMadsen]

Another unintended consequence I was thinking of: if triggerAsyncId is set in a child AsyncResource without keeping a reference to the parent AsyncResource object. Then the parent emitDestroy will be called while the parent resource is still producing other resources.

function parent() {
  const resource = new AsyncResource('parent')
  // setup resource
  return resource.asyncId;
}

function child(triggerAsyncId) {
  const resource = new AsyncResource('child', triggerAsyncId)
  // setup resource
}

const triggerAsyncId = parent()
// parent::emitDestroy could be called
//
// ... some time later
child(triggerAsyncId)

This is only an issue if the user holds only on the asyncId. If the user holds the parent resource object instead then the resource won't be gc'ed while being used.

[Sebmaster]

@AndreasMadsen Very good point, but since asyncIds are all Numbers instead of transparent objects like what's returned from setImmediate, I don't think there's a way to properly solve that really.

There're two ways I see here:

• Assume that the triggerAsyncId param is basically an extension of the sensitive embedder api (by making asyncIds "useful" as non-transparent comparison objects) and remove it with the sensitive embedder api.
• Just add a config option to opt AsyncResources out of GC tracking.

Any other ways to solve that / opinions on these two ^?