Closed Qard closed 4 years ago
The InternalCallbackScope
function trampoline idea seems to get rather complicated with C++ streams and promises. I'm thinking I might rearrange things a bit to rely more on wrapping callbacks on the JS side, focusing on things like the non-handle requests callbacks, and seeing where I can go from there. As it is currently, InternalCallbackScope
overlays too much functionality deeper into the stack in many places that changing it to receive a callback and arguments would need fairly far-reaching changes. I also think it would likely be rendered somewhat redundant by efforts to wrap the callbacks before even passing it into the C++ side. I think I'm going to skip that one for now. π€
Thanks a lot for working on this with that a high focus!
In general I would prefer correctness over performance which would pull the last point to the first. But I fully understand that it is done the other way around as the last item has a hard dependency to v8. Therefore it's difficult to progress and most likely hard to get it done within the time-frame you mentioned. If we could at least get some inputs from V8 team how to progress in this would be good.
Many thanks for describing these improvements!
I'm specifically interested in improving AsyncLocalStorage
+ native promises performance (section "Avoid wrapping promises with AsyncWrap if destroy is not used"). Please, let me know if there is something that can be done to help you here.
Great initiative! Thank you very much for starting this!
I'm interested in wrapping argumets for callbacks, if this can help: if we start tracking from callback then all it's argumments can be wrapped too, on JS side, I can show examples if necessary. It is an easiest way to controll execution path. The same idea is chaining for .then
and in deep. The hardiest thing to controll is when some function creates function inside and then pushes it somewhere, IDK can it be tracked or not. And the final step is Async Constructor, when we have await new Constuctor
, though it is well tracked by promises, but there can be nested topics if this would be class
instead of async function
, because inheritance chain is not the same for classes.
I think it is connected to #375.
Also here is possible solution for synchronous callback tracking, thought it shouldn't be done this way, because it is solution for userland impl.
@Qard As far as InternalCallbackScope
is concerned, one possible idea might be to only optimize in the case that the (Internal)MakeCallback()
utility is being used. We could pass kSkipAsyncHooks
to the callback scope to make it not emit the event, then call the trampoline function instead?
@Flarna Correctness is definitely one of my priorities. The ultimate goal is to ensure that async_hooks is both fast and reliable, correctness being part of reliability. Currently promises are the main thing interfering with it being fast and thenables are one of the main things intefering with it being reliable so those are two primary areas of focus in my plans here. If you can think of anything else to investigate which falls into either of those categories, I'm happy to have a look.
@wentout I'd be interested to see any examples of your thinking there. My basic idea is to wrap callbacks assigned to the oncomplete
field on request types. Just as simple as req.oncomplete = wrap(asyncId, callback)
where the wrap
function basically just does this:
function wrap (asyncId, callback) {
return function (...args) {
hooks.before(asyncId)
callback.apply(this, args)
hooks.after(asyncId)
}
}
It'd step over the whole JavaScript to native jump and just go directly to the JavaScript-side event router.
@addaleax Yep, MakeCallback
was my motivation for the idea. Core seems to rely more on the scope form to hold the context through a deeper stack though. If the stack was shallower it might be reasonable but it gets fairly deep with C++ streams. It's also fully blocked by native promises, so those at least would need to keep the scope-based separated before and after. I'll re-evaluate it later though. π€
I've re-arranged things a bit to focus first on not binding handlers that aren't used and on avoiding wrapping promises. I'll get back to the JavaScript-side function trampoline and wrapper ideas later. They'll probably be influenced by the changes to split up handler bindings anyway.
If you can think of anything else to investigate which falls into either of those categories, I'm happy to have a look.
@Qard Unfortunatelly there is one more place that I have in mind: napi_make_callback has support for async_context
but this works different for async_ids then for async resources. see also https://github.com/nodejs/node/pull/32063#discussion_r386953680
Please don't take this as task given by me. And in general feel free to ping me if you need some more infos/help even I can't promise a specific amount of time to spend.
Another topic which was also controversal in the past was which execution context to link through Promises. Currently execution path is passed from Promise creation to then/catch callbacks. Another option would be to link the context where then
/catch
is called instead creation. For a lot usecases this is anyway the same.
But honeslty speaking I'm not sure if we should revisit this. Just want to have it here for completness.
@Qard
I'd be interested to see any examples of your thinking there...:
Sharing the same idea my own outdated CLS implementations covers bit more. Please look for answer
variable below:
function wrap (asyncId, callback) {
return function (...args) {
hooks.before(asyncId);
const answer = callback.apply(this, args);
hooks.after(asyncId);
if (typeof answer === 'function') {
// might be here is necessary to wrap answer again
// to finish the execution path... but it depends on impl
}
return answer;
}
}
Keeping at lease return answer;
should help to fit the gap for userland.
It'd step over the whole JavaScript to native jump and just go directly to the JavaScript-side event router.
As I understood, sorry, this would be amazing. If we had a way to see the synchronous split as a separate but sequenced part of asyn_hook IDs, this would cover everything for the User Mode Queuing problem... In an attempt to solve it for business purpose we did a patch for require module with Acorn AST walker to wrap each function with the example above. But performance was so low... so I choosed the other scenario for making code in userland. And that other scenario then followed to the async prototype chain (await new Constructor
) and so on, but this still requires CLS as a part of tracing the IDs for underlying code.
And here is a proposal for Async Init of @bmeck
I think this might be connected with this initiative in future if there would be a way to properly figure out of async subclassing. But async constructors for plain function are possible right now, when we return promise from constructor, which then resolves to this
: this is an async prototype chain... And this all is already covered for tracing purpose by AsyncLocalStorage from 26540.
Here are slides of Continuation idea.
Meaning, if this is possible, then of splitted Trie of the execution path made by splitting the asyncIDs of synchronous context done by native jump might solve this known traking~tracing problem, when CLS context was missing because topology sorting is not applicable (because there is a hole made by synchronous split).
Avoid wrapping promises with AsyncWrap if destroy is not used
... To deal with this issue, I'm considering a few options. ... I'm also considering an additional option passed alongside the hooks object to indicate that destroy events are wanted for consumable resource but not for promises--possibly even a filter/subscription API of some sort to explicitly describe which things to listen to or not.
Can it be as simple as a bitset (i.e. an integer value) available on the native side where each bit means presence of any active hooks of a specific type? This bitset could be changed in lib/internal/async_hooks.js
each time when hooks are enabled/disabled. Also, destroy
hook presence check in PromiseHook
function will be fast (a single bitwise operation) and if there are no active destroy
hooks, there would be no need to wrap Promise
with PromiseWrap
and assigned async id can be stored as an internal field directly in the Promise
. This should decrease the overhead of ALS + native promises combination, while keeping the same overall async_hooks
behavior (except for resource in init
hook not being a PromiseWrap
, but rather Promise
itself).
P.S. I'm not good at the native side of async_hooks
, so take what's written above with a grain of salt.
Currently working on changing the PromiseHook handler to avoid needing PromiseWrap. Even if the destroy hook is not registered though, it currently still needs to track the destroy to call EmitTraceEventDestroy
. There's also the isChainedPromise
property on the wrapped resource to consider. Not sure what the best way to handle this would be, but my hope is that I can just move the entire PromiseHook handler into JavaScript and re-implement the wrap object on the JS side, maybe. I could just use the existing registerDestroyHook
to track the destroy for the object only if there's a destroy handler listening for it. π€
@Qard
Currently working on changing the PromiseHook handler to avoid needing PromiseWrap. Even if the destroy hook is not registered though, it currently still needs to track the destroy to call
EmitTraceEventDestroy
.
What is the purpose of calling EmitTraceEventDestroy
when there are no destroy
hooks?
There's also the
isChainedPromise
property on the wrapped resource to consider.
I know that it's a part of public API, but what's the purpose of having it? It's possible to track parent async resource via triggerAsyncId
and check if it's a Promise (although, there may be false positives with the approach in cases when Promises are nested, not chained - but that is a rare case).
Not sure what the best way to handle this would be, but my hope is that I can just move the entire
PromiseHook
handler into JavaScript and re-implement the wrap object on the JS side, maybe. I could just use the existingregisterDestroyHook
to track the destroy for the object only if there's a destroy handler listening for it.
The idea sounds really promising IMO. This way we could use AsyncResource
on JS side, which already skips destroy
emit when possible (see #32429).
How about building a PoC and benchmarking it just to make sure that we get the performance improvement before going any further with a proper implementation? Let me know if there is something I can help you with here.
EmitTraceEventDestroy
is for the trace_events
API, not async_hooks
. Not calling it would be a breaking change, I think.
The isChainedPromise
is used by zone.js to identify if a promise has a parent promise or not. I don't know the specifics of why, but they've said before that they need it.
As for the AsyncResource
comment, that's basically what I was looking at doing, though I currently have my own separate similar class specific to PromiseHook, because I don't have a callback to wrap but instead distinct before and after events. The AsyncResource
class is also currently in the public file rather than the internal file, and the promise hook stuff needs to exist in the internal file, so I'd have to move a bunch of stuff around. For now, I'm just trying to get it to work as a direct port, then I'll see what I can do about switching it to AsyncResource
or something like that. I currently have the JS interface exposed and mostly working already, but having some stack corruption issues I'll be investigating today.
@Qard
EmitTraceEventDestroy
is for thetrace_events
API, notasync_hooks
. Not calling it would be a breaking change, I think.
As far as I can see, trace_events
create an async hook when async_hooks
category is enabled. See:
https://github.com/nodejs/node/blob/a9da65699a43f81076a1560e09ae97ad5630c35f/lib/internal/trace_events_async_hooks.js#L31
So, it should be safe to rely on active destroy
hooks count when deciding whether we need to emit destroy
event or not, without considering trace_events
module's state.
The
isChainedPromise
is used by zone.js to identify if a promise has a parent promise or not. I don't know the specifics of why, but they've said before that they need it.
Hmm, I've checked current code base of zone.js and it doesn't seem to be using async_hooks
. Instead it seems to deal with monkey patching:
https://github.com/angular/angular/tree/master/packages/zone.js/lib/node
So, I wonder if there are any users of isChainedPromise
? More context: this method was renamed in #18633 a couple of years ago, which was a breaking change.
I currently have the JS interface exposed and mostly working already, but having some stack corruption issues I'll be investigating today.
That's awesome! I'll be waiting for any updates from you.
That's not what I was referring to with trace_events. There's a separate EnvPromiseHook
scope for the native AsyncWrap lifecycle events in the existing PromiseHook handler.
Not too familiar with the current state of zone.js, so perhaps it's a non-issue. In any case, it's a very edge-casey feature within an already edge-casey feature and it's all marked as experimental so it's probably not a big deal if we just drop it. I'll just see what direction this stuff takes as I progress. π€·
I've opened a draft PR with the current state of the code here: https://github.com/nodejs/node/pull/32891
It's not working yet, but demonstrates the general idea. (Though very much not performance-tuned)
That's not what I was referring to with trace_events. There's a separate
EnvPromiseHook
scope for the native AsyncWrap lifecycle events in the existing PromiseHook handler.
Can't we keep that EnvPromiseHook
trace scope in PromiseHook
and move to JS side Promise trace_events
tracking (so that Promise tracking happens with the async hook installed by trace_events
)?
Update. Looks like that's the way how you did it in https://github.com/nodejs/node/pull/32891, so ignore this question.
It needs the destroy event too though. It creates a span for the callback by starting and ending at the before and after events, but it also creates a span around around the resource lifetime by starting and ending on the init and destroy events. If there is no destroy then that span will never be ended and I'm not sure what the implications of that would be for trace_events. Since it's supposed to just be an event log, it might be fine to just be missing an event.
@jasnell Any insight here?
yep, you're exactly right @Qard, not having visibility on the destroy event would be a disaster for tools like clinic.js that specifically analyze object lifetimes.
If there is no destroy then that span will never be ended and I'm not sure what the implications of that would be for trace_events. Since it's supposed to just be an event log, it might be fine to just be missing an event.
But if trace_events
's async_hooks category is enabled, we automatically have an active destroy
hook. So, the question is whether it's fine to have a missing destroy
event in situations when trace_events
were enabled programmatically after a Promise was created.
Currently trace events will not be emitted for promises at all unless the PromiseHook is enabled. What I'm thinking is maybe only emitting them if there is a destroy hook.
What we could do is have enablePromiseHooks
accept an optional callback. If a callback is given, it runs that as it does in the PR I'm working on currently. If no callback is given, it would just fallback to the existing native PromiseHook handler. This way we could have a lighter-weight handler when no destroy handler is present.
Could also just have two handlers in C++ and have a separate function like enablePromiseHooksWithDestroys
or something like that to start the heavier tracking. π€
Proposal for reworking promise integration into async_hooks:
Regarding the promise story, I think the current strategy is fundamentally wrong. Right now the promise lifecycle is somehow intertwined with async_hooks
in a way that causes all issues mentioned above.
edit: clarified that each `[[then]]
call gets its own unique resource.
I think the right approach is to instead rework promise integration into async_hooks
such that.
[[then]]
on a promise
or thenable
create a resource object (or use the promise/thenable object created by [[then]]
) then call the init
hook.[[then]]
callback, the before
hook is called.[[then]]
callback call the after
hook, immediately after call the destroy
hook.This avoids tracking promise objects with the garbage collector, directly solving "Avoid wrapping promises with AsyncWrap if destroy is not used" and might make "Make PromiseHook support thenables" more feasible.
edit: Solving "Make PromiseHook support thenables" is more feasible because we don't need to know when the object was created or destroyed. Only when it is used by native JS APIs that invokes the microtask queue. That is actually doable! Only manual calls to [[then]]
on a thenable, will not be tracked. But I don't see that as a concern, because that is not an async action.
In terms of still providing insight into a promise lifecycle, a dedicated promise_hooks
module should be created. A user can then connect the promise lifecycle with async_hooks
via a promiseId
that is exposed both in promise_hook
and via the HandleObject in async_hooks
.
This does change the default async stack, as a chain of promises would become shorted to just the last promise. I think for most purposes this is actually desired, and a user can recover the full stack by integrating promise_hooks
.
Finally, I want to highlight that right now, tracking the actual async boundary that [[then]]
creates via its microtask is actually very difficult, and when multiple .then()
calls are used on the same promise it is actually impossible. This will be fixed by this proposal, without removing features.
PS: I know I have haven't been active in node.js for a long time. I still want to participate if you think it can be helpful on very specific issues, but the mail-storm I receive via @nodejs/collaborators
makes it impossible to notice those issues. You are always welcome to email me if you want some direct input.
- at the call of
[[then]]
in apromise
orthenable
, theinit
hook is called.
What happens if a user-land module calls new Promise()
in one async context, but the resolve
happens in another one (say, some operation queuing is happening)? With the proposed change AsyncLocalStorage
(and, probably, user-land CLS libraries) may loose context.
- at the end of the
[[then]]
callback, theafter
hook is called followed by thedestroy
hook.
This may be too early to emit destroy
here as at this point init
emit didn't happen for the chained promise, if any. I have a feeling that destroy
hook based CLS implementations, like cls-hooked
, may loose context with this change.
I didn't check the above considerations in practice, so I may be wrong.
The real problem, in my opinion, is that we are conflating promise gc with handle closes in the destroy hook. Those should really be two different events that can be handled separately, and that's one of the things I hope to work on.
- at the call of
[[then]]
in apromise
orthenable
, theinit
hook is called.What happens if a user-land module calls
new Promise()
in one async context, but theresolve
happens in another one (say, some operation queuing is happening)? With the proposed changeAsyncLocalStorage
(and, probably, user-land CLS libraries) may loose context.
Resolve happens usually in a differnt context and I don't think this is the point here. I think it's about the following:
const p = new Promise()
is called in context 1p.then(cbA)
is called in context 2 and p.then(cbB)
in context 3
If the usecase is context propagation: Would you expect to link the then cbA with Context1 or Context2?I agree with @AndreasMadsen that usually propagation from the place where then
is called is "more correct". In a lot cases (e.g. if await is used) it's anyway the same.
But I'm not sure if this can be event implemented with current v8 APIs.
I wouldn't say await
is quite the same. You can eager-init.
async function main() {
const first = asyncThing()
const second = asyncThing()
await first
await second
}
The point at which the Promise
instance is created for second
is different from when the await
and therefore the then
happens.
What happens if a user-land module calls
new Promise()
in one async context, but theresolve
happens in another one (say, some operation queuing is happening)? With the proposed changeAsyncLocalStorage
(and, probably, user-land CLS libraries) may loose context.Resolve happens usually in a differnt context and I don't think this is the point here.
No, the point here is different. Notice the "some operation queuing is happening" part. There multiple popular libraries that don't play nicely with async_hooks
, mainly, because of user-land queuing of internal operations. But when users promisify them, the problem vanishes, as native promises act just like AsyncResource
. With the proposed changes, all of such applications may be broken.
So, the question here is whether native Promise
should behave identical to AsyncResource
(as it is now), or we want it to have a completely different behavior? I'm afraid that in the latter case we risk breaking user applications.
I should point out that is exactly the timing issue that this PR is solving. In async/await, the [[then]]
is called from within an unwrapped microtask, so there is currently no async_hooks
stack to link to. Moving all promises to this model would definitely break everything.
at the call of [[then]] in a promise or thenable, the init hook is called. What happens if a user-land module calls new Promise() in one async context, but the resolve happens in another one (say, some operation queuing is happening)? With the proposed change AsyncLocalStorage (and, probably, user-land CLS libraries) may loose context.
They will not lose context, they will have a different context. If that context is unsatisfying, one should integrate with promise_hooks
. But I think in most cases it is satisfying.
at the end of the[[then]] callback, the after hook is called followed by the destroy hook. This may be too early to emit destroy here as at this point init emit didn't happen for the chained promise, if any. I have a feeling that destroy hook based CLS implementations, like cls-hooked, may loose context with this change.
The relevant conversation isn't if it is too early to emit destroy
but if it is too early to emit after
. destroy
only signals that before
will never be emitted again, which is by spec true.
You are right that some context is lost. I honestly don't think it matters. Let's say you have:
function context() { // context: root
new Promise(fnStart)
.then(fnA) // context: root -> A
.then(fnB) // context: root -> B
.then(fnC) // context: root -> C
}
Then yes you are correct you longer don't get root -> A -> B -> C
. But in terms of async stack traces, and CLS the context is sufficient.
They will not lose context, they will have a different context. If that context is unsatisfying, one should integrate with
promise_hooks
. But I think in most cases it is satisfying.
I see. You propose to emit init
and then
is called, not its callback. In this case, multiple then
calls will lead to multiple init
emits, which contradicts with current async_hooks
behavior which assumes that init
hook is called once per async resource.
I'm also aware that multiple calls to then
may be problematic with the current code base in certain situations.
Considering this, do you propose to assign a unique async id to each invocation of then
?
You are right that some context is lost. I honestly don't think it matters. Let's say you have:
function context() { // context: root new Promise(fnStart) .then(fnA) // context: root -> A .then(fnB) // context: root -> B .then(fnC) // context: root -> C }
This will be a problem if a new context scope is started in the middle of the chain. In essence, we loose information about the async chain here, as the correct chain is root -> A -> B -> C
.
Don't get me wrong. I'm all for simplifying things, but we need to avoid breaking existing code, at least without a really strong reason. Maybe it makes sense to introduce promise_hooks
as a separate experimental module and keep async_hooks
as they are now? The new module could rethink async_hooks
lifecycle: it could remove destroy
hook, for instance.
In this case, it will be possible to introduce an analog of AsyncLocalStorage
(or a special mode of the current implementation) that will be promise_hooks
based. This CLS implementation will work in any applications that deal with promise based APIs only and, by design, should have smaller overhead than the current AsyncLocalStorage
's one.
Considering this, do you propose to assign a unique async id to each invocation of then?
Yes. Each then
is its own async resource, with its own async id. A Promise
object is not the actual async boundary, and therefore not what should be tracked by async_hooks
. The async boundary is the microtrask caused by .then()
.
Don't get me wrong. I'm all for simplifying things, but we need to avoid breaking existing code, at least without a really strong reason. Maybe it makes sense to introduce promise_hooks as a separate experimental module and keep async_hooks as they are now? The new module could rethink async_hooks lifecycle: it could remove destroy hook, for instance.
I think we do need to break the existing code. As I see it there are 3 issues with promises:
destroy
hook is not called if async_hook
is enabled after Promise creation.I don't think any of these issues can be solved given the current async_hooks
specifications around Promises.
This will be a problem if a new context scope is started in the middle of the chain. In essence, we loose information about the async chain here, as the correct chain is root -> A -> B -> C.
I disagree that the "correct chain" is root -> A -> B -> C
. I think there are multiple correct chains. This is also discussed in the old Microsoft paper on the subject.
I just want to point out that the current default context is not root -> A -> B -> C
:
const { AsyncLocalStorage } = require('async_hooks');
const cls = new AsyncLocalStorage();
cls.run({ s: ['root'] }, function () {
Promise.resolve(1)
.then((v) => cls.run({ s: [...cls.getStore().s, 'A'] },
() => console.log(cls.getStore().s)))
.then((v) => cls.run({ s: [...cls.getStore().s, 'B'] },
() => console.log(cls.getStore().s)))
.then((v) => cls.run({ s: [...cls.getStore().s, 'C'] },
() => console.log(cls.getStore().s)))
});
prints [ 'root', 'A' ]
, [ 'root', 'B' ]
, [ 'root', 'C' ]
. And not [ 'root', 'A' ]
, [ 'root', 'A', 'B' ]
, [ 'root', 'A', 'B', 'C' ]
.
root -> A -> B -> C
would be an optional context path that can be implemented given the right promise_hooks
API.Maybe it makes sense to introduce promise_hooks as a separate experimental module and keep async_hooks as they are now? The new module could rethink async_hooks lifecycle: it could remove destroy hook, for instance.
No, that would just make a bad situation worse. async_hooks
is already mixed with promise specific semantics, such as resolve
. Mixing promise_hooks
with async_hooks
just makes things worse.
I agree that the then(...)
call is the most "correct" representation of the async barrier and we should therefore be creating a resource at that level. However, because await runs that in an unwrapped microtask that would break everything, the way things are currently. We would need a way to be able to capture and wrap the context for each microtask for it to run as within the queue. Perhaps we could supply our own custom MicrotaskQueue
?
We would need a way to be able to capture and wrap the context for each microtask for it to run as within the queue. Perhaps we could supply our own custom MicrotaskQueue?
Makes sense. I think that would also be a good approach for hooking into the native Promise APIs to support thenables.
This will be a problem if a new context scope is started in the middle of the chain. In essence, we loose information about the async chain here, as the correct chain is root -> A -> B -> C.
- I disagree that the "correct chain" is
root -> A -> B -> C
. I think there are multiple correct chains. This is also discussed in the old Microsoft paper on the subject.- I just want to point out that the current default context is not
root -> A -> B -> C
:
Yes, you're right. Both chains can be thought as "correct". And the current behavior is root -> A, root -> B, root -> C
because of intermediate promises which have root promise as the parent.
No, that would just make a bad situation worse.
async_hooks
is already mixed with promise specific semantics, such asresolve
. Mixingpromise_hooks
withasync_hooks
just makes things worse.
Makes sense. Sounds like a change that could be shipped in the next major release.
The only potential problem that I can foresee is the fact that AsyncLocalStorage
currently stores the context in a symbol property of the promise wrap (or the promise itself, if we consider #32891). With the proposed change a single promise may stand for multiple async resources, yet the context is stored in a single place. Maybe we could fix that with a wrap per each then
invocation, but it may lead to races because of the asynchronous execution of the callback. WDYT?
Sounds like a change that could be shipped in the next major release.
Yes, this would be a breaking change. So definitely for next major release.
The only potential problem that I can foresee is the fact that AsyncLocalStorage currently stores the context in a symbol property of the promise wrap (or the promise itself, if we consider #32891). With the proposed change a single promise may stand for multiple async resources, yet the context is stored in a single place. Maybe we could fix that with a wrap per each then invocation, but it may lead to races because of the asynchronous execution of the callback. WDYT?
I don't really understand. There will still be an object to attach properties too, you could even make it the promise object returned by .then()
. The callbacks will be executed in the order they are queued, no difference there. It is also possible that using object properties will be less necessary since this would meditate a lot of the performance issues with destroy
.
I don't really understand.
Update. The snippet below was updated, as the initial one wasn't correct.
I'm speaking of timing issues like the following one:
const als = new AsyncLocalStorage();
const aPromise = new Promise((resolve, reject) => {
setTimeout(resolve, 0);
});
als.run(1, () => {
aPromise.then(() => { // store 1 gets propagated here
console.log(als.getStore()); // 2 - Incorrect, should be 1
});
als.run(2, () => {
aPromise.then(() => { // store 2 gets propagated here and overwrites store 1
console.log(als.getStore()); // 2 - Correct
});
});
});
The above snippet assumes current implementation of AsyncLocalStorage
and the proposed Promise + async_hooks
integration. It's a bit synthetic and, yes, the current code base may also lead to weird behavior in edge cases (yet, they're different). But my point is that a single property in combination with the proposed behavior may lead to certain problems and it should be improved somehow during the change.
There will still be an object to attach properties too, you could even make it the promise object returned by
.then()
.
Yes, in any case we should have a separate resource object per init
emit.
It is also possible that using object properties will be less necessary since this would meditate a lot of the performance issues with
destroy
.
AsyncResource
still uses GC-tracking by default. Also, additional hooks have a certain overhead themselves. So, destroy
-hook based CLS implementations may still be slower even after the discussed change.
The above snippet assumes current implementation of AsyncLocalStorage and the proposed Promise + async_hooks integration. It's a bit synthetic and, yes, the current code base may also lead to weird behavior in edge cases (yet, they're different). But my point is that a single property in combination with the proposed behavior may lead to certain problems and it should be improved somehow during the change.
This is a problem with AsyncLocalStorage
. I was not here for when it was made, so I don't know the background for why it was designed the way it is. As far as I can see, .run()
introduces a memory leak, which is also what leads to the bug you mention.
In any case, the issue you describe is no different if process.nextTick
was used. Therefore, it has nothing to do with my proposed change to Promise integration in aynnc_hooks
.
const als = new AsyncLocalStorage();
als.run(1, () => {
process.nextTick(() => { // store 1 gets propagated here
console.log(als.getStore()); // 2 - Incorrect, should be 1
});
als.run(2, () => {
process.nextTick(() => { // store 2 gets propagated here and overwrites store 1
console.log(als.getStore()); // 2 - Correct
});
});
});
This is a problem with
AsyncLocalStorage
. I was not here for when it was made, so I don't know the background for why it was designed the way it is. As far as I can see,.run()
introduces a memory leak, which is also what leads to the bug you mention.
What makes you think it's a mem leak? In fact ALS is more memory safe than destroy
-based CLS implementations, as it relies on GC of async resources.
In any case, the issue you describe is no different if
process.nextTick
was used. Therefore, it has nothing to do with my proposed change to Promise integration inasync_hooks
.const als = new AsyncLocalStorage(); als.run(1, () => { process.nextTick(() => { // store 1 gets propagated here console.log(als.getStore()); // 2 - Incorrect, should be 1 }); als.run(2, () => { process.nextTick(() => { // store 2 gets propagated here and overwrites store 1 console.log(als.getStore()); // 2 - Correct }); }); });
I've just tried running this snippet on node 14.2.0 and got 1
and 2
printed in console, which is the expected behavior. That happens because each process.nextTick
invocation is tracked as a separate async resource, so propagation works correctly in this code.
So, my original snippet, which was showing a potential problem with the proposed promise integration and current ALS implementation, still looks like a valid one.
What makes you think it's a mem leak? In fact ALS is more memory safe than destroy-based CLS implementations, as it relies on GC of async resources.
I could be wrong. That is anyway a separate discussion.
I've just tried running this snippet on node 14.2.0 and got 1 and 2 printed in console, which is the expected behavior. That happens because each process.nextTick invocation is tracked as a separate async resource, so propagation works correctly in this code.
That is great. If it works with process.nextTick
it will work with .then
. My proposed change makes them behave the same. edit: well, similar to queueMicrotask()
.
I could be wrong. That is anyway a separate discussion.
Agreed.
That is great. If it works with
process.nextTick
it will work with.then
. My proposed change makes them behave the same.
With the proposed behavior in promise integration, the original snippet (the one with a promise) will produce incorrect results. Luckily, it may be fixed by having a separate resource object per then
invocation. That's just something that should be kept in mind when implementing your proposal.
With the proposed behavior in promise integration, the original snippet (the one with a promise) will produce incorrect results. Luckily, it may be fixed by having a separate resource object per then invocation. That's just something that should be kept in mind when implementing your proposal.
Great. Please don't assume anymore that I intended to call the [[init]]
multiple times on the same resource. That was never the case and it leads to a strawman discussion that is a waste of time.
I have clarified in the initial proposal that a new resource object should be created, or alternatively the returned the Promise by [[then]]
is used as the resource.
I have clarified in the initial proposal that a new resource object should be created, or alternatively the returned the Promise by
[[then]]
is used as the resource.
Good to know. Thanks.
Does it make sense to move your proposal to a separate GH issue?
@puzpuzpuz Alright, I wrote my proposal as its own issue. See: https://github.com/nodejs/diagnostics/issues/389
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
Over the next several months I plan to work on async_hooks improvements full-time. I have several experiments I intend to try, so I will document my plans here and link to the PRs as I progress. I'm not sure how much of this I'll be able to finish in that time, but this is my rough plan and likely priority order. Let me know if you have any questions or suggestions for other things to look at.
Avoid binding all event handlers when only some are used
The native side of async_hooks was designed to only handle one set of hooks at a time and the JavaScript side abstracts that with a design which shares one native hook set across many JavaScript hook sets. It currently will bind all hook functions even if some are never used.
This is particularly a problem because the new
AsyncLocalStorage
class intended for context management aims to improve performance over traditional init/destroy-based context management by using only the init hook and theexecutionAsyncResource
function. This allows it to avoid using the destroy hook, but the internals continue to bind all the other methods even though they are not being used.Avoid wrapping promises with AsyncWrap if destroy is not used
A deeper issue of
executionAsyncResource
existing as an alternative to the destroy hook is that the primary performance penalty of the destroy hook is due to needing to wrap every single promise in anAsyncWrap
instance in order to track its memory and trigger the destroy hook when it is garbage collected. If the destroy hook is not even being listened to, all that overhead of wrapping the promise, tracking the garbage collection, and queuing destroy events for the next tick (because you can't call JavaScript mid-gc) becomes completely unnecessary.This issue specifically is currently the largest performance hit of turning on async_hooks in most apps. Eliminating this would be huge.
To deal with this issue, I'm considering a few options. The main one being to expose PromiseHook events directly to JavaScript in a separate API from async_hooks and then having async_hooks join those events into its own event stream purely on the JavaScript side, making use of WeakRef to trigger the destroy events only if there is a destroy hook listening for them. I'm also considering an additional option passed alongside the hooks object to indicate that destroy events are wanted for consumable resource but not for promises--possibly even a filter/subscription API of some sort to explicitly describe which things to listen to or not.
InternalCallbackScope function trampoline
status: on hold - C++ streams and promises make this complicated. Will re-evaluate later.
Currently "before" and "after" events of async_hooks are largely emitted on the C++ side from within the InternalCallbackScope class. It is currently designed in such a way that the "before" and "after" handlers of a hook are triggered by separate entrances into JavaScript. I want to try and create a function trampoline, similar to how timers works, which would involve passing the callback and arguments to a JavaScript function which would trigger the "before" event fully on the JS-side, then call the callback, then trigger the "after" event. This would reduce three separate entrances into the JavaScript layer down to just one. It also opens the door for the next two ideas...
Avoid hop to C++ from AsyncResource events
Currently AsyncResource, which expresses JavaScript side pseudo-asyncrony such as connection pooling mechanics, sends events into the C++ side of async_hooks despite that data only actually being useful to consume on the JavaScript side. That barrier hop is unnecessary and only exists because async_hooks was designed with the expectation that everything is wrapped in a C++ type which is passed through
InternalCallbackScope
to trigger the lifecycle emitter functions on the wrapper class.Wrap non-handle request callbacks
In libuv, there are "handles" expressing long-term resources like a socket or file descriptor and there are "requests" to express single-use things. Some request objects are constructed within C++ from handle functions, however many are constructed in JavaScript where it would be possible to wrap the callback assigned to the
oncomplete
field to trigger the before and after hooks, enabling to bypass the native side of async_hooks entirely.Bonus Round - Make PromiseHook support thenables
One major stumbling block with context management in Node.js is PromiseHook does not currently support thenables, an object which has a
then(...)
function on it but is not a real promise. Thenables are commonly used in database and cache libraries, among other things, which breaks every async_hooks user. There are some complex hacks to work around it, but they are complicated, fragile, and depend heavily on sequence timing/ordering of the microtask queue, which is quite risky. PromiseHook needs to support thenables too.I don't have experience contributing to V8 myself, though I'm relatively familiar with the internals as a compilers enthusiast that enjoys spelunking in the codebase now and then. I could use some assistance here from anyone with experience contributing to V8 and making Node.js builds with custom V8 builds. π