async_hooks: Add destroy event for gced AsyncResources #16998

Sebmaster · 2017-11-13T20:34:55Z

In cases where libraries create AsyncResources which may be emitting
more events depending on usage, the only way to ensure that destroy is
called properly is by calling it when the resource gets garbage
collected.

Fixes: #16153

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

python tools/test.py -J --mode=release async-hooks/test-callback-error fails for me on master as well as this branch...

Affected core subsystem(s)

doc, async_hooks

/cc @addaleax

Sebmaster · 2017-11-13T20:35:36Z

lib/async_hooks.js

@@ -259,8 +262,15 @@ function validateAsyncId(asyncId, type) {

 // Embedder API //

+const undestroyedAsyncIds = new Set();


Are Sets a thing in lib/, or is that too much of a problem for perf?

It’s perfectly fine.

What’s the reason for this, though? Avoiding double cleanup when the implementor emits destroy manually? Can you add a comment?

Exactly. Will do.

If possible, I would like to avoid yet another global state and yet another special emit function :)

Is it not possible to read a property from the handle object in MakeCallback? Then we could just have another symbol property on in the handle object that indicates if emitDestroy was called manually.

Another option is forcing the user to either always call emitDestroy() or otherwise never call emitDestroy(), depending on the optional flag is also an option.

Ahh I see what you mean. That should be possible; I think v8 doesn't clear up the object before the weak callback gets invoked. I'll implement that.

What are the other use-cases?

I was thinking about resources which allow explicit disposal, like imagine a connection where you could call conn.close() or just let it fall out of scope. I think the expectation there would be for the destroy event to emitted at the time of the close call rather than at GC time (necessitating a way to prevent a double call).

After thinking about it some, that would not allow us to set the symbol on AsyncResource if the asyncId is destroyed by the sensitive API (since we don't have an AsyncResource reference available there).

Does there currently not exist a map with all async ids where we could store state in at all? I thought until now that async_id_fields did so we could shove an additional field in there, but just realized that that's just global state management after looking at it.

Don't worry about the sensitive API. We will be deprecating that anyway and I think it is perfectly fine to require the user call emitDestroy manually if they insist on using a low-level API.

Does there currently not exist a map with all async ids where we could store state in at all?

No such map exists.

Unfortunately, after testing it, it seems like v8 frees the object before the weakcallback is called, so we can't attach props there either 😞

Here we go, found a solution by introducing another object into it for a level of indirection. 🎉

That good with you?

Sebmaster · 2017-11-13T20:36:36Z

src/async-wrap.h

@@ -157,6 +157,9 @@ class AsyncWrap : public BaseObject {

  virtual size_t self_size() const = 0;

+  class DestroyParam;


I think we either have to leave this public, or move RegisterDestroyHook into AsyncWrap. Since the other registered methods don't do that, I wasn't sure if that'd work?

Without having looked too much, you can assume that any code behind NODE_WANT_INTERNALS (including this header file) is not considered public

addaleax · 2017-11-13T20:41:44Z

src/async-wrap.cc

+  v8::Persistent<Function> callback;
+};
+void AsyncWrap::WeakCallback(
+  const v8::WeakCallbackInfo<AsyncWrap::DestroyParam> &info


We generally use left-leaning pointers and references, i.e. … DestroyParam>& info.

addaleax · 2017-11-13T20:42:06Z

src/async-wrap.cc

+  double asyncId;
+  v8::Persistent<Function> callback;
+};
+void AsyncWrap::WeakCallback(


Can you leave two lines of space between definitions? Otherwise this is very dense code :)

addaleax · 2017-11-13T20:42:35Z

src/async-wrap.cc

@@ -407,6 +407,40 @@ static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
 }


+class AsyncWrap::DestroyParam {


I don’t think this needs to be on AsyncWrap, it’s kind of it’s own different thing (it can stay in this class though)

You mean just dropping up(/down?) into the node namespace, but keep it in the same file?

Yes, exactly :)

addaleax · 2017-11-13T20:44:21Z

src/async-wrap.cc

+
+  Environment* env = Environment::GetCurrent(args);
+
+  v8::Persistent<Object> target(env->isolate(), args[0].As<Object>());


I think this might be causing trouble because there’s no way to reset the persistent if it’s stack-allocated? Maybe that’s not an issue, but I’d prefer it if this was a property on DestroyParam

We're only using this to register the weak callback and let it get cleaned up afterwards. Do we need to keep it alive for v8?

It’s making me a bit uneasy at least in the sense that there are comments in v8.h that hint at a future change towards v8::Persistent resetting itself when the destructor is called (which I would welcome, I think):

node/deps/v8/include/v8.h

Lines 633 to 634 in b204b09

* At present kResetInDestructor is not set, but that will change in a future

* version.

AndreasMadsen · 2017-11-13T20:59:39Z

lib/async_hooks.js

 class AsyncResource {
-  constructor(type, triggerAsyncId = initTriggerId()) {
+  constructor(type, triggerAsyncId = initTriggerId(), opts = {}) {


Given that that triggerAsyncId is also optional. I think the signature should be:

// won't actually work constructor(type, options = { triggerAsyncId = initTriggerId(), disableTracking: true })

For backward compatibility, we can check with typeof options === 'number' and also deprecate that usage.

AndreasMadsen · 2017-11-13T21:02:22Z

lib/async_hooks.js

@@ -278,6 +288,11 @@ class AsyncResource {
    emitInitScript(
      this[async_id_symbol], type, this[trigger_async_id_symbol], this
    );
+
+    if (!opts.disableTracking) {


Tracking/trace can mean so many things. Maybe call it if (options.manualDestroy)?

Hmm, we don't actually mind manualDestroy since we prevent double destroys. What we actually dislike is a delayed (after GC) sensitive emitDestroy. Soo delayedSensitiveDestroy? May be too long...

I don’t mind long names (I actually think they are a great idea and we need more of them), but delayedSensitiveDestroy doesn’t seem to say much imo … how about requireManualDestroy if you want to be explicit about what is optional/required?

Sounds good to me.

AndreasMadsen · 2017-11-13T21:18:03Z

src/async-wrap.cc

+  delete p;
+
+  Local<v8::Primitive> undef = Undefined(info.GetIsolate());
+  fn->Call(


I'm not entirely sure how this SetWeek works but we have had issues before, where calling into JavaScript when the GC is running caused a process crash. This is why we have PushBackDestroyAsyncId.

Did you delete your other comment? I think it'd be possible to do this rather easily, if we save a Map<AsyncId, destroyManual> in JS, but I can't quite figure out how MakeCallback works for the other thing you mentioned.

It got collapsed #16998 (comment)

If make it such that it calls emitNativeDestroy directly (some suggestions in #16998 (comment)) then it should be straightforward to use PushBackDestroyAsyncId.

PushBackDestroyAsyncId basically just calls emitNativeDestroy when it is safe.

AndreasMadsen · 2017-11-14T08:35:36Z

doc/api/async_hooks.md

@@ -566,11 +568,14 @@ asyncResource.asyncId();
 asyncResource.triggerAsyncId();
 ```

-#### `AsyncResource(type[, triggerAsyncId])`
+#### `AsyncResource(type[, triggerAsyncId [, options]])`


remember to update documentation

jasnell · 2017-11-14T19:12:05Z

doc/api/async_hooks.md

-  * `disableTracking` {boolean} Disables automatic `emitDestroy` when the
-  object is garbage collected.  **Default:** `false`
+  * `requireManualDestroy` {boolean} Disables automatic `emitDestroy` when the
+  object is garbage collected. You usually do not need to set this option (even


Please avoid using informal pronouns like you in the docs.

Fishrock123 · 2017-11-14T23:05:53Z

src/async-wrap.cc

+  p->propBag.Reset(env->isolate(), args[2].As<Object>());
+
+  p->target.SetWeak(
+    p, AsyncWrap::WeakCallback, v8::WeakCallbackType::kParameter);


Isn't this stuff rather perf-costly?

Just checked this with hrtime a bit:

It appears that while not fast there's a bigger problem:
The .As<Object>() are very slow during instantiation of AsyncResource. Just leaving p->target.Reset(isolate, args[0].As<Object>()); in the Register, moves us from 0.2s for 5MM instantions to 2.4s. Is there any way to avoid the v8 ::Cast for that?

.As<T> is just an unchecked cast; it essentially compiles down to zero instructions at run-time. It's the new and .Reset() calls that are slow.

The .SetWeak() call itself isn't very slow but it creates more work for the garbage collector.

AndreasMadsen · 2017-11-15T12:24:04Z

lib/async_hooks.js

    this[destroyedSymbol] = { destroyed: false };

    emitInitScript(
      this[async_id_symbol], type, this[trigger_async_id_symbol], this
    );

    if (!opts.requireManualDestroy) {
-      registerDestroyHook(this, this[async_id_symbol], this[destroyedSymbol]);
+      registerDestroyHook(this, this[async_id_symbol], this[destroyedSymbol], "destroyed");


Typically we solve the performance penalty of creating a string in C++ by having it as a global. We have a lot of those so don't feel bad about adding another one: https://github.com/nodejs/node/blob/master/src/env.h#L100

AndreasMadsen · 2017-11-15T12:25:30Z

doc/api/async_hooks.md

@@ -543,12 +543,14 @@ will occur and the process will abort.
 The following is an overview of the `AsyncResource` API.

 ```js
-const { AsyncResource } = require('async_hooks');
+const { AsyncResource, triggerAsyncId } = require('async_hooks');


Maybe this should be executionAsyncId, to match the code below?

AndreasMadsen · 2017-11-15T13:16:13Z

@Sebmaster it sounds like you did a benchmark of AsyncResource, could you make that benchmark available in benchmark/async_hooks?

Even if there is a performance penalty it might still be fine. AsyncResource is only used by userland so it doesn't have to be super fast and we have a fast option using requireManualDestroy too. I would just like to know much of an effect this has :)

edit: documentation for our benchmark suite can be found here: https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md

Sebmaster · 2017-11-15T13:28:00Z

@AndreasMadsen Is there a way to specify expose-gc in the benchmark? If not I'm probably just gonna compare instantiation rather than also including gc runtime.

AndreasMadsen · 2017-11-15T13:32:00Z

@Sebmaster yes, you can actually do it with the flags option. example: https://github.com/nodejs/node/blob/master/benchmark/http2/simple.js#L16

Sebmaster · 2017-11-15T13:34:02Z

Sweet. Added.

Results for me:

async_hooks/gc-tracking.js method="trackingEnabled" n=1000000: 1,165,678.3064871693
async_hooks/gc-tracking.js method="trackingDisabled" n=1000000: 18,171,028.88563058

AndreasMadsen · 2017-11-15T13:41:51Z

I wish I could remember how to read that 😂 . I think you can run:

node benchmark/scatter.js benchmark/async_hooks/gc-tracking.js > scatter.csv
cat scatter.csv | Rscript benchmark/scatter.R --xaxis n --category method

See: https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md#comparing-parameters

Unfortunately, we don't have a CI job for parameter comparing :/

Sebmaster · 2017-11-15T13:56:25Z

Here you go:

      n           method     rate confidence.interval
 1000000 trackingDisabled 17949587           640123.52
 1000000  trackingEnabled  1170751            21084.96

AndreasMadsen · 2017-11-15T14:04:37Z

@Sebmaster Thanks . That is definetly a big difference (15x). If we can't do anything to improve this then we should consider documenting that requireManualDestroy: true is faster. But I also think a rate of 1170751 ops/s = 0.8 µs/ops is fine.

AndreasMadsen

LGTM.

@Sebmaster I'm very impressed by your work, it is quite a complex change for a first-time contribution. You have been extremely responsive and understood all of our feedback, which often is a big challenge.

Be proud :)

addaleax

LGTM, with a few comments

nice work!

addaleax · 2017-11-15T20:01:33Z

src/async-wrap.cc

+
+  Environment* env = Environment::GetCurrent(info.GetIsolate());
+  DestroyParam* p = info.GetParameter();
+  Local<Object> propBag = PersistentToLocal(info.GetIsolate(), p->propBag);


Tiny nit: prop_bag for local variables (See https://github.com/nodejs/node/blob/master/CPP_STYLE_GUIDE.md :) )

addaleax · 2017-11-15T20:03:32Z

src/async-wrap.cc

+  Local<Object> propBag = PersistentToLocal(info.GetIsolate(), p->propBag);
+
+  Local<Value> val = propBag->Get(env->destroyed_string());
+  if (!val->BooleanValue()) {


if (val->IsFalse())? Otherwise you’ll need to switch to the variant of BooleanValue() that is not deprecated (i.e. the one that takes a Local<Context> context argument)

addaleax · 2017-11-15T20:04:37Z

src/async-wrap.cc

+
+  Isolate* isolate = args.GetIsolate();
+  DestroyParam* p = new DestroyParam();
+  p->asyncId = args[1]->NumberValue();


Can you use args[1].As<Number>()->Value()? This is deprecated (and slower for when we don’t have val->IsNumber() == true).

addaleax · 2017-11-15T20:06:16Z

src/async-wrap.cc

+  if (!val->BooleanValue()) {
+    PushBackDestroyAsyncId(env, p->asyncId);
+  }
+  p->propBag.Reset();


Can you maybe CHECK(p->target.IsEmpty()); too, just to be sure?

That fails. I'm pretty sure the pointer just becomes invalid, but still points somewhere.

Hm, in that case, I would assume the pointer is still valid – but either way, calling .Reset() on it should be fine?

Oh, that works. I was pretty sure that failed previously, but owell, apparently misremembering.

addaleax · 2017-11-15T20:48:25Z

Okay, I think we’re all set here! 👍

Fresh CI: https://ci.nodejs.org/job/node-test-commit/14057/

addaleax · 2017-11-15T20:53:15Z

I was almost getting ready to paste the linter failure here 😄 I don’t think we need to kick off another CI run for this.

AndreasMadsen · 2017-11-16T11:16:05Z

I need to feel safe: https://ci.nodejs.org/job/node-test-pull-request/11471/

addaleax · 2017-11-16T11:31:20Z

@Sebmaster Sorry, it looks you might have to rebase this now that #15538 has landed … :(

In cases where libraries create AsyncResources which may be emitting more events depending on usage, the only way to ensure that destroy is called properly is by calling it when the resource gets garbage collected. Fixes: #16153

Sebmaster · 2017-11-16T12:27:57Z

Done. Building took its sweet time 😁

AndreasMadsen · 2017-11-16T13:00:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11474/

AndreasMadsen · 2017-11-16T14:25:33Z

One issue, but it is just a flaky test: #14568

addaleax · 2017-11-16T19:08:23Z

Landed in 22901d8, thanks for the PR! 🎉

In cases where libraries create AsyncResources which may be emitting more events depending on usage, the only way to ensure that destroy is called properly is by calling it when the resource gets garbage collected. Fixes: #16153 PR-URL: #16998 Fixes: #16153 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Sebmaster · 2017-12-07T20:04:18Z

@addaleax Is there a process for getting this into the current release branches, or is this a v10-only thing?

addaleax · 2017-12-07T20:11:54Z

@Sebmaster This should be picked up automatically, I think the only issue is that nobody in @nodejs/release has done a 9.x release in a while?

I don’t currently have the bandwidth to prepare one myself, sorry

Sebmaster · 2017-12-07T20:15:38Z

Ah, that's fine, just saw that 9.2.0 came out after the merge and wasn't sure if it just didn't make the cut.

evanlucas · 2017-12-07T21:56:28Z

Hm it may have had conflicts when I cut v9.2.0. I'll dig into it after v9.2.1 and try to get it in a release next Tuesday-ish

In cases where libraries create AsyncResources which may be emitting more events depending on usage, the only way to ensure that destroy is called properly is by calling it when the resource gets garbage collected. Fixes: #16153 PR-URL: #16998 Fixes: #16153 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

In cases where libraries create AsyncResources which may be emitting more events depending on usage, the only way to ensure that destroy is called properly is by calling it when the resource gets garbage collected. Fixes: nodejs#16153 PR-URL: nodejs#16998 Backport-PR-URL: nodejs#18179 Fixes: nodejs#16153 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

In cases where libraries create AsyncResources which may be emitting more events depending on usage, the only way to ensure that destroy is called properly is by calling it when the resource gets garbage collected. Backport-PR-URL: #18179 Fixes: #16153 PR-URL: #16998 Fixes: #16153 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 13, 2017

Sebmaster commented Nov 13, 2017

View reviewed changes

jasnell requested review from trevnorris, AndreasMadsen and addaleax November 13, 2017 20:37

addaleax reviewed Nov 13, 2017

View reviewed changes

AndreasMadsen reviewed Nov 13, 2017

View reviewed changes

mscdex added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap labels Nov 13, 2017

AndreasMadsen reviewed Nov 13, 2017

View reviewed changes

AndreasMadsen reviewed Nov 14, 2017

View reviewed changes

jasnell reviewed Nov 14, 2017

View reviewed changes

Fishrock123 reviewed Nov 14, 2017

View reviewed changes

AndreasMadsen reviewed Nov 15, 2017

View reviewed changes

AndreasMadsen approved these changes Nov 15, 2017

View reviewed changes

addaleax approved these changes Nov 15, 2017

View reviewed changes

async_hooks: add destroy event for gced AsyncResources

fb7d126

In cases where libraries create AsyncResources which may be emitting more events depending on usage, the only way to ensure that destroy is called properly is by calling it when the resource gets garbage collected. Fixes: #16153

jasnell approved these changes Nov 16, 2017

View reviewed changes

addaleax closed this Nov 16, 2017

Sebmaster deleted the feature/async_hooks-destroy-on-gc branch November 22, 2017 15:21

AndreasMadsen added dont-land-on-v4.x labels Dec 7, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

AndreasMadsen added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 12, 2017

AndreasMadsen added the lts-watch-v8.x label Jan 16, 2018

This was referenced Jan 16, 2018

Semver Minor Tracking nodejs/Release#298

Closed

Backport async hooks to v8.x #18179

Closed

Jeff-Lewis mentioned this pull request Mar 18, 2018

Clean old context references Jeff-Lewis/cls-hooked#21

Open

devoto13 mentioned this pull request Feb 16, 2023

Emit destroy hook when AsyncResource has fulfilled its purpose stream-utils/raw-body#86

Closed

		@@ -259,8 +262,15 @@ function validateAsyncId(asyncId, type) {

		// Embedder API //

		const undestroyedAsyncIds = new Set();

		@@ -157,6 +157,9 @@ class AsyncWrap : public BaseObject {

		virtual size_t self_size() const = 0;

		class DestroyParam;

		@@ -407,6 +407,40 @@ static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
		}


		class AsyncWrap::DestroyParam {


		Environment* env = Environment::GetCurrent(args);

		v8::Persistent<Object> target(env->isolate(), args[0].As<Object>());

	* At present kResetInDestructor is not set, but that will change in a future
	* version.

async_hooks: Add destroy event for gced AsyncResources #16998

async_hooks: Add destroy event for gced AsyncResources #16998

Conversation

Sebmaster commented Nov 13, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

AndreasMadsen commented Nov 15, 2017 • edited Loading

Sebmaster commented Nov 15, 2017

AndreasMadsen commented Nov 15, 2017

Sebmaster commented Nov 15, 2017

AndreasMadsen commented Nov 15, 2017 • edited Loading

Sebmaster commented Nov 15, 2017

AndreasMadsen commented Nov 15, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Nov 15, 2017

addaleax commented Nov 15, 2017

AndreasMadsen commented Nov 16, 2017

addaleax commented Nov 16, 2017

Sebmaster commented Nov 16, 2017

AndreasMadsen commented Nov 16, 2017

AndreasMadsen commented Nov 16, 2017

addaleax commented Nov 16, 2017

Sebmaster commented Dec 7, 2017

addaleax commented Dec 7, 2017

Sebmaster commented Dec 7, 2017

evanlucas commented Dec 7, 2017

Sebmaster commented Nov 13, 2017 •

edited

Loading

AndreasMadsen Nov 13, 2017 •

edited

Loading

AndreasMadsen Nov 13, 2017 •

edited

Loading

AndreasMadsen Nov 14, 2017 •

edited

Loading

AndreasMadsen Nov 15, 2017 •

edited

Loading

AndreasMadsen commented Nov 15, 2017 •

edited

Loading

AndreasMadsen commented Nov 15, 2017 •

edited

Loading