async_hooks: use resource objects for Promises #13452

addaleax · 2017-06-04T12:25:08Z

As discussed in #13367. /cc @nodejs/async_hooks

Use PromiseWrap resource objects whose lifetimes are tied to
the Promise instances themselves to track promises, and have
a .promise getter that points to the Promise and a .parent
property that points to the parent Promise’s resource object,
if there is any.

The properties are implemented as getters for internal fields
rather than normal properties in the hope that it helps keep
performance for the common case that async_hooks users will
often not inspect them.

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

Affected core subsystem(s)

async_hooks

AndreasMadsen · 2017-06-04T13:28:21Z

Thanks for looking at this.

Do we not risk getting a very long chain of PromiseWrap objects that prevents all the promises in the chain from being easily garbage collected?

I was thinking it might be better to use the asyncId of the parent, rather than the PromiseWrap itself. If the user want to walk the .parent chain they can just store the information in a Map.

There is also the problem of resource.parent.parent being misleading if the PromiseHooks were enabled late. By returning the parent id it will always be correct and if the user enables PromiseHooks late they just don't have the information to walk the .parent chain, which is a reasonable consequence.

/cc @JiaLiPassion

AndreasMadsen · 2017-06-04T13:30:59Z

Also, cc @Fishrock123 who was the first to request this.

JiaLiPassion · 2017-06-04T14:22:36Z

@addaleax , thank you for implementing this.

@AndreasMadsen , are you suggestion to pass the asyncId of the parent promise in init hook like below?

const parent_promise = new Promise((resolve, reject) => {resolve(5);}); 
const promise = parent_promise.then((val) => {return val;});

function init(id, type, triggerId, handle) {
    process._rawDebug(`id: ${id}, type: ${type}, triggerId: ${triggerId}, 
                                        handle.promise: ${handle.promise}, 
                                        handle.parentid: ${handle.parentId}`);
}

the output will look like

id: 2, type: PROMISE, triggerId: 1, handle.promise: {status: 'pending'}, handle.parentid: undefined
id: 3, type: PROMISE, triggerId: 2, handle.promise: {status: 'pending'}, handle.parentid: 2

And if application want the promise object, they can keep a asyncId and promise map
in the application memory themselves.
Is my understanding correct?

AndreasMadsen · 2017-06-04T14:45:16Z

Is my understanding correct?

Yes. That is exactly what I mean.

JiaLiPassion · 2017-06-04T14:58:25Z

@AndreasMadsen , got it! I still try to understand the C++ code,
If this can make GC easier to control, I agree this is a less-risky solution.

addaleax · 2017-06-04T15:02:45Z

@AndreasMadsen @JiaLiPassion Updated!

I still try to understand the C++ code,

If you have any questions, please ask them, here or in the diff – whatever you want to know, a lot of other people will also like to know.

JiaLiPassion · 2017-06-04T15:07:04Z

If you have any questions, please ask them, here or in the diff – whatever you want to know, a lot of other people will also like to know.

@addaleax , got it!

AndreasMadsen · 2017-06-04T15:09:37Z

src/async-wrap.cc

+    promise_wrap_template->SetAccessor(
+        FIXED_ONE_BYTE_STRING(env->isolate(), "parentId"),
+        PromiseWrap::GetParentId);
+    env->SetProtoMethod(ctor, "getAsyncId", AsyncWrap::GetAsyncId);


What is the cost of this? If there is any I don't think we should add it. The result is given as the first parameter in init anyway and we generally don't document that getAsyncId exists.

I thought it’s nice to have for consistency, but removing it is trivial. I’ve removed it for now, we can always revisit if necessary.

I haven’t measured, but I can’t really imagine it’s costly – it adds a method to the prototype, which should be practically free.

AndreasMadsen · 2017-06-04T15:11:34Z

src/async-wrap.cc

+  Local<Object> resource_object;
+  PromiseWrap* wrap = nullptr;
+  if (resource_object_value->IsObject()) {
+    resource_object = resource_object_value.As<Object>();


My V8-C++ knowledge might be lacking, but why do we check that resource_object_value is an Object and then cast it to an Object.

.As<Object>() is not a cast in the JS sense of the word; you can read it as telling the engine “I’ve made sure that this is an object, now can I please use it as one?” (i.e. calling handle.As<T>() is undefined behaviour if handle is not actually a T)

Ah, similar to reinterpret_cast.

In addition a Debug build it asserts, so instead of simply crashing b/c it was cast to the wrong type it'll actually say you've screwed up.

AndreasMadsen

Code LGTM. Since zone.js will be dependent on this, I think should document the PromiseWrap resource object.

addaleax · 2017-06-04T15:34:38Z

@AndreasMadsen I’ve added some documentation… PTAL

AndreasMadsen · 2017-06-04T15:37:23Z

doc/api/async_hooks.md

@@ -203,13 +203,16 @@ UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST,
 RANDOMBYTESREQUEST, TLSWRAP
 ```

+There is also the `PROMISE` resource type, which is used to track `Promise`


Maybe we should add PROMISE to the provider list in async_wrap.

I’ve kept it out of it because it doesn’t quite qualify as “provided by the built-in Node.js modules”, but if you think it’s better, I can just drop this sentence and reword the existing text a bit

No just leave it out for now. @trevnorris suggested that maybe we should add all types to the Providers list. See #13287 (comment)

Hm. Seems we don't even include Timer or TickObject in that list. Or even as a side comment.

Anyway, only argument I'd have for adding it to the above list is because it shows up in process.binding('async_wrap').Providers. But that's not a strong argument. I'll leave this up to you.

I think we should delete the list. The user shouldn't really on a defined list of types because the Embedder API allows for extra types.

@AndreasMadsen The reason I decided to pass the "type" (history note: using Provider was not the name I wanted to use, but switched long ago b/c of pressure to change on the PR) was for to allow filtering resources. I'd like to have a list of all "core types" (which would include TickObject and Timer) and I'd expect module authors to document their own "types".

Though because this relies a bit on internals could we put a note like "subject to change in future major (or minor?) releases without deprecation"?

We already have a note saying resource objects are not to be trusted:

... The API for getting this information is currently not considered public ...

I guess we can be more specific.

JiaLiPassion · 2017-06-04T15:43:53Z

src/async-wrap.cc

@@ -282,37 +285,87 @@ bool AsyncWrap::EmitAfter(Environment* env, double async_id) {
 class PromiseWrap : public AsyncWrap {
 public:
  PromiseWrap(Environment* env, Local<Object> object, bool silent)
-    : AsyncWrap(env, object, PROVIDER_PROMISE, silent) {}
+    : AsyncWrap(env, object, PROVIDER_PROMISE, silent) {
+    MakeWeak(this);


@addaleax , sorry for very basic question, why we add

MakeWeak(this);

in this PR, since now we only keep the asyncId of parent promise here, is there any GC issue if we don't call MakeWeak ?

This question is not basic – it’s something I didn’t quite figure out until a couple days ago myself. And I’m not sure whether you’ve seen it, but the current MakeWeak call is being removed here, too: https://github.com/nodejs/node/pull/13452/files#diff-5e552c79e1538215f1621d1774852e71L315

The thing is, MakeWeak is a not the best choice of name. What it actually does is to set the 0th internal field of the associated object to the passed pointer, and then mark the handled contained in the Wrap object as weak and set a destroy callback for V8 to call.

And it needs to then make handle weak, because otherwise all V8 knows is that there is a persistent handle to the object; it doesn’t know how we use it, so it can’t garbage collect it. By making it weak, we tell V8 to destroy it once there are no other (non-weak) handles left, and to run our registered destroy callback (which will delete the C++ instance of PromiseWrap that we are using).

I agree, it’s all a bit tricky to see through. I’m mostly moving it here because that helps with avoiding duplication, and because that’s how we use it elsewhere in the code as well.

@addaleax , thank you for the explanation. I just read MakeWeak API and node_object_wrap.h, and I will continue to learn about it.

And I am not quite understand is why we don't need the MakeWeak call before this version?

And I am not quite understand is why we don't need the MakeWeak call before this version?

It was there, it was just called after the constructor instead of being a part of it: https://github.com/nodejs/node/pull/13452/files#diff-5e552c79e1538215f1621d1774852e71L315

@addaleax , oh, yes, it was there, thanks. Currently I have no further questions!

@addaleax

The thing is, MakeWeak is a not the best choice of name. What it actually does is to set the 0th internal field of the associated object to the passed pointer, and then mark the handled contained in the Wrap object as weak and set a destroy callback for V8 to call.

And... I'm to blame for this one. In fe2df3b I carefully went through to make sure Wrap() and ClearWrap() were called for all applicable instances forgetting that 3.5 years ago for whatever forsaken reason when I implemented BaseObject (d120d92) I made MakeWeak() automatically Wrap() the object. This was a horrible mistake on my part

Basically I would be happy if we removed the call to Wrap() in BaseObject::MakeWeak().

@JiaLiPassion

I just read MakeWeak API and node_object_wrap.h, and I will continue to learn about it.

Be careful. AsyncWrap doesn't use node_object_wrap. It uses base-object

@trevnorris ,

Be careful. AsyncWrap doesn't use node_object_wrap. It uses base-object

thank you for pointing out this one.

AndreasMadsen

LGTM.

I set it to semver-minor since this is an API change, although technically we make less than experimental guarantees about the resource object, so an argument could be made for semver-patch.

I think we should remove the type list. The user shouldn't really on a defined list of types because the Embedder API allows for extra types. However, it is not related to the PR.

refack · 2017-06-07T18:06:35Z

I set it to semver-minor since this is an API change, although technically we make less than experimental guarantees about the resource object, so an argument could be made for semver-patch.

I think we should remove the type list. The user shouldn't really on a defined list of types because the Embedder API allows for extra types. However, it is not related to the PR.

👍

(@AndreasMadsen If you put comments in the "review summary" I can't 👍 them)

AndreasMadsen · 2017-06-07T19:17:56Z

(@AndreasMadsen If you put comments in the "review summary" I can't 👍 them)

I know. I prefer it this way :D evil laughter

addaleax · 2017-06-07T19:22:26Z

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

trevnorris · 2017-06-07T20:44:14Z

src/async-wrap.cc


 static void PromiseHook(PromiseHookType type, Local<Promise> promise,
                        Local<Value> parent, void* arg) {
  Local<Context> context = promise->CreationContext();
  Environment* env = Environment::GetCurrent(context);
-  PromiseWrap* wrap = Unwrap<PromiseWrap>(promise);
+  Local<Value> resource_object_value = promise->GetInternalField(0);
+  Local<Object> resource_object;


nit: this is only used in the if statement below so mind just making it:

Local<Object> resource_object = resource_object_value.As<Object>();

Use `PromiseWrap` resource objects whose lifetimes are tied to the `Promise` instances themselves to track promises, and have a `.promise` getter that points to the `Promise` and a `.parent` property that points to the parent Promise’s resource object, if there is any. The properties are implemented as getters for internal fields rather than normal properties in the hope that it helps keep performance for the common case that async_hooks users will often not inspect them.

addaleax · 2017-06-08T18:37:42Z

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

trevnorris

Thank you much

addaleax · 2017-06-08T22:01:23Z

Landed in 8f39881

Use `PromiseWrap` resource objects whose lifetimes are tied to the `Promise` instances themselves to track promises, and have a `.promise` getter that points to the `Promise` and a `.parent` property that points to the parent Promise’s resource object, if there is any. The properties are implemented as getters for internal fields rather than normal properties in the hope that it helps keep performance for the common case that async_hooks users will often not inspect them. PR-URL: #13452 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Ref: nodejs#13452

PR-URL: #13561 Ref: #13452 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use `PromiseWrap` resource objects whose lifetimes are tied to the `Promise` instances themselves to track promises, and have a `.promise` getter that points to the `Promise` and a `.parent` property that points to the parent Promise’s resource object, if there is any. The properties are implemented as getters for internal fields rather than normal properties in the hope that it helps keep performance for the common case that async_hooks users will often not inspect them. PR-URL: #13452 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #13561 Ref: #13452 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Use `PromiseWrap` resource objects whose lifetimes are tied to the `Promise` instances themselves to track promises, and have a `.promise` getter that points to the `Promise` and a `.parent` property that points to the parent Promise’s resource object, if there is any. The properties are implemented as getters for internal fields rather than normal properties in the hope that it helps keep performance for the common case that async_hooks users will often not inspect them. PR-URL: #13452 Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

nodejs-github-bot added async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 4, 2017

addaleax added async_hooks Issues and PRs related to the async hooks subsystem. promises Issues and PRs related to ECMAScript promises. labels Jun 4, 2017

AndreasMadsen reviewed Jun 4, 2017

View reviewed changes

JiaLiPassion reviewed Jun 4, 2017

View reviewed changes

JiaLiPassion mentioned this pull request Jun 5, 2017

[WIP] feat(async): fix #740, use async hooks to wrap nodejs async api angular/zone.js#795

Open

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

AndreasMadsen approved these changes Jun 7, 2017

View reviewed changes

trevnorris reviewed Jun 7, 2017

View reviewed changes

addaleax added 5 commits June 8, 2017 20:34

[squash] provide parent id rather than parent resource

0875682

[squash] remove getAsyncId

464d2af

[squash] add docs

2652910

[squash] address nit

4acaed3

addaleax force-pushed the promise-wrap-resource branch from ec1c0d4 to 4acaed3 Compare June 8, 2017 18:36

trevnorris approved these changes Jun 8, 2017

View reviewed changes

addaleax closed this Jun 8, 2017

addaleax deleted the promise-wrap-resource branch June 8, 2017 22:01

addaleax mentioned this pull request Jun 8, 2017

doc: update async_hooks providers list #13561

Closed

2 tasks

addaleax added a commit to addaleax/node that referenced this pull request Jun 10, 2017

doc: update async_hooks providers list

4bbad95

Ref: nodejs#13452

addaleax mentioned this pull request Jun 10, 2017

v8.1.1 proposal #13598

Merged

jasnell pushed a commit that referenced this pull request Jun 13, 2017

doc: update async_hooks providers list

0ab4614

PR-URL: #13561 Ref: #13452 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Jun 17, 2017

doc: update async_hooks providers list

4298f9e

PR-URL: #13561 Ref: #13452 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

addaleax added a commit that referenced this pull request Jun 21, 2017

doc: update async_hooks providers list

9b74dde

PR-URL: #13561 Ref: #13452 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

async_hooks: use resource objects for Promises #13452

async_hooks: use resource objects for Promises #13452

Conversation

addaleax commented Jun 4, 2017 • edited Loading

Checklist

Affected core subsystem(s)

AndreasMadsen commented Jun 4, 2017 • edited Loading

AndreasMadsen commented Jun 4, 2017 • edited Loading

JiaLiPassion commented Jun 4, 2017 • edited Loading

AndreasMadsen commented Jun 4, 2017

JiaLiPassion commented Jun 4, 2017

addaleax commented Jun 4, 2017

JiaLiPassion commented Jun 4, 2017

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 Jun 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen left a comment

Choose a reason for hiding this comment

addaleax commented Jun 4, 2017

AndreasMadsen Jun 4, 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

addaleax Jun 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jun 4, 2017 • edited Loading

Choose a reason for hiding this comment

trevnorris Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen left a comment • edited Loading

Choose a reason for hiding this comment

refack commented Jun 7, 2017

AndreasMadsen commented Jun 7, 2017 • edited Loading

addaleax commented Jun 7, 2017

Choose a reason for hiding this comment

addaleax commented Jun 8, 2017

trevnorris left a comment

Choose a reason for hiding this comment

addaleax commented Jun 8, 2017

addaleax commented Jun 4, 2017 •

edited

Loading

AndreasMadsen commented Jun 4, 2017 •

edited

Loading

AndreasMadsen commented Jun 4, 2017 •

edited

Loading

JiaLiPassion commented Jun 4, 2017 •

edited

Loading

AndreasMadsen Jun 4, 2017 •

edited

Loading

AndreasMadsen Jun 4, 2017 •

edited

Loading

addaleax Jun 4, 2017 •

edited

Loading

JiaLiPassion Jun 4, 2017 •

edited

Loading

trevnorris Jun 6, 2017 •

edited

Loading

AndreasMadsen left a comment •

edited

Loading

AndreasMadsen commented Jun 7, 2017 •

edited

Loading