async_hooks: for promises is PromiseWrap::parentId actually useful?

[ofrobots]

On PromiseWrap objects we expose a parentId property:

node/src/async_wrap.cc

Lines 385 to 387 in 0993fbe

	promise_wrap_template->SetAccessor(
	FIXED_ONE_BYTE_STRING(env->isolate(), "parentId"),
	PromiseWrap::getParentAsyncId);

.

The semantics of the property are:

• If the promise has a parent promise, parentId will be the asyncId of the parent promise. By definition, and by implementation, parentId must match the triggerAsyncId available in the Init hook, as the parent promise is the trigger (cause) of the child async resource (promise) to be created. The parentId is redundant information in this case.
• The only case where parentId would be different from triggerAsyncId is when a promise has no parent. In this case the parentId will be undefined and the triggerAsyncId would correspond to the current execution id.

It seems the only information that parentId provides is whether or not (i.e boolean) the promise has a parent.

Is there something more subtle here that I might be missing? I'd like to go ahead and document that if that's the case.

/cc @nodejs/diagnostics @nodejs/async_hooks

[AndreasMadsen]

@ofrobots Yes, the point of parentId is to see if the promise has a parent. It is something the https://github.com/angular/zone.js team are depending on.

/cc @JiaLiPassion

[JiaLiPassion]

@ofrobots , zone.js are trying to make an asynchooks version for better performance and handle async/await.

As @AndreasMadsen said, in older version of zone.js, zone.js try to use parentId to check promise is a chained promise or not.

With the current implementation, I use the promise from resource, https://github.com/JiaLiPassion/zone.js/blob/adcb3600dad36a3f4e979c17a05cba1bdd6b99fc/lib/node/async_hooks.ts#L128, so I can check whether the promise is a chained promise or not. For current implementation, parentId may not necessary, but if the promise from resource is gone in the future, I will still need parentId.

[ofrobots]

Thanks for the explanation @JiaLiPassion.

The reason I opened this issue is because async_hooks are quite complex conceptually, and when reading the docs, I would expect users to have questions about how parentId would relate to the triggerAsyncId and executionAsyncId. Based on the discussion so far, it seems that the parentId might be completely unnecessary, or maybe we should replace it with a 'isChainedPromise' boolean property on the PromiseWrap object. That would reduce the cognitive complexity of the API.

@AndreasMadsen WDYT?

[JiaLiPassion]

@ofrobots , currently zone.js not only need to know the promise is chained or not, but also need to monkey-patch the promise itself if it is chained. so with current implementation zone.js still need to promise object.

The reason zone.js need the promise object is to handle native async/await interception. without the promise object, it is not possible to intercept native async/await.

Thank you!

[ofrobots]

I am not proposing anything be done with the promise on the resource at this point :). Only that we should change the parentId field.

[JiaLiPassion]

@ofrobots , got it. As you said

• the triggerAsyncId is parentId when promise is chained
• the triggerAsyncId is the executionId when promise is not chained.

So I agree we only need to expose a isChainedPromise instead of a duplicate parentId.

[AndreasMadsen]

Yes, I think isChainedPromise would be better.

[ofrobots]

Given that there is merely a cognitive burden of having too many concepts, I decided not to make a breaking change for this (even though async_hooks is experimental). I think explaining this in the docs addresses the issue. #18633