Multiple users of asyncWrap in a single application?

[mike-kaufman]

Hi -

I'm not clear on how two independent modules using asyncWrap will co-exist in the same application.Is this a supported scenario?

My current understanding is setupHooks() will overwrite any existing callbacks. Consequently two modules independently using asyncWrap and linked into the same app will conflict and only one module will work correctly. Is this an accurate understanding? Has this been discussed?

Thanks much!

Mike

[AndreasMadsen]

You are correct that AsyncWrap only support one setupHooks() call and will conflict with other modules.

It is something we are aware of, but it is not yet decided if it is something we want to support. There is some discussion here: nodejs/Release#59 (comment)

[mike-kaufman]

Thanks Andreas. I'm not clear on the results of what was discussed by the TSC on this topic. Can you elaborate? Going through the thread you cite above, I'm having a hard time following the reasoning for not fixing. Assuming this is a valuable & useful API (and I believe it is), then it clearly follows that multiple libraries will start using it. Not supporting multiple callers in the same process is clearly broken. Am I missing something?

Thanks,

Mike

[mike-kaufman]

@trevnorris - would love to hear your reasoning on this.

[AndreasMadsen]

I'm not clear on the results of what was discussed by the TSC on this topic.

The discussion in nodejs/Release#59 was about the stability of AsyncWrap and if it was acceptable to land breaking changes in an LTS release. Thus not the appropriate place to discuss the finer details about the future of conflict management for AsyncWrap. This is actually something we haven't done yet.

@trevnorris was at the time (and may still be) primarily against supporting multiple users for the sake of keeping the complexity in node core low. But I will let Trevor elaborate on that.

I still hope to support multiple users. I suggest you look at https://github.com/AndreasMadsen/async-hook and give us (me) feedback about the API. That way we can get an idea about how much is needed in an multi-user implementation, if we decide to implement that in node core.

[mike-kaufman]

So, yeah, async-hooks is almost exactly the API I was expecting to see from a client point-of-view. IMO it makes sense to pull this (or something analogous) as the public API surface here.

Only difference between what you have and what I was expecting is I expected something like:

var cookie = asyncHooks.add(...);  // returns an identifier that uniquely identifies my set of callbacks
asyncHooks.remove(cookie);  // allows for deterministic and constant-time de-registration of my callbacks.

Minor feedback:

• index.js, 21-17 - not clear to me why you're doing this.
• I'm not clear on why you need to monkey-patch timers, promises, nextTick. Probably my ignorance.
• async-hook.js, 83-88. First concern here is this is a linear scan (maybe this is OK). Second concern here is that if two callers (say c & c+2) register the same function (e.g., same init(), different pre()), and then caller c+2 calls remove(), the ordering of callbacks is no longer pairwise. That is, the assumptions that initFns[i], preFns[i], etc are all the callbacks from the same call to add() breaks. Not sure if this even matters, or how realistic a scenario this is, but it stands out to me.

[AndreasMadsen]

@mike-kaufman yep. Point 1 and 2 should not be a concern in a node core implementation. However point 3 highlights some of the complexities when supporting multiple users (another is the global effect of .disable()). Are these issues acceptable or is a more complex implementation required? Keep in mind that AsyncWrap is perhaps the hottest code path in node, thus API is not the only concern.

[mike-kaufman]

So, I think point 3 is a minor point. You can probably ship what you have and my guess is you'll be fine. If it does prove to be problematic, you would likely need to introduce new APIs to keep a deterministic grouping of callbacks.

Now, you could fix this relatively easily today by using single hash table that maps cookie->set of callbacks (in lieu of storing callbacks in four distinct arrays). You'll want to be careful in how you choose cookies, as my guess is you want to keep the order of callback execution deterministic, specifically ith caller if add() is the ith to have their callbacks invoked. You could just choose cookies in a monotonically increasing sequence (0, 1, 2...).

I'm assuming in above statement, by "hot", you mean performance sensitive, and I'm assuming the hot path here is callback invocation path and not the add/remove path. Let me know if not the case, but I don't think storing callbacks as mentioned above would impact perf in any meaningful way. Would love to know why if not the case.

Last bit of feedback - for safety, consider wrapping callback invocations inside a try/catch, eg. here. If a mis-behaving callback throws, it will prevent all other hooks from running.

[avanderhoorn]

If it helps we have had similar discussions around some of these concerns (i.e. having multiple listeners and the cost/complexity it adds) in .net. The resolution that we came to was that if the code is optimized for 0 or 1 listeners, then that fits the majority of real world use cases but it should still support multiples. If the user decided to add packages which results in more than 1 listeners than that is a decision that they have made. Otherwise it limits a whole number of use cases and pushes the responsibility to authors using asyncwrap as to how they will try and play nicely with others.

[chrisdickinson]

Would there be a downside to implementing the multiple listeners approach at the JS level, and leaving the internal C++ implementation single-listener?

[mike-kaufman]

@chrisdickinson - as an end-user, all I care about is if I register callbacks, they get called. :) As an end-user, I don't care whether this is implemented in JS or native. If implemented in JS (e.g., async-hook), then I would hope that the async-wrap setupHooks() API is hidden such that it can't be abused, and there's one official JS module that supports add/remove of multiple callbacks.

I'm curious to know if there are perf concerns that would suggest implementing at the native level.

[mike-kaufman]

Or perhaps this API is useful to other native modules? And that is a reason to implement in c++?

[AndreasMadsen]

@chrisdickinson there will be an extra function evaluation which likely will affect performance and there will be an extra callSite in error.stack.

I did play around with it, to measure the performance (AndreasMadsen/node@dc1403f...AndreasMadsen:cbbc48e), but discovered that node doesn't provide the necessary benchmark tooling (statistical significance) to say much about it. But I'm currently fixing that.

[AndreasMadsen]

@mike-kaufman By the way, the reason why async-hooks needs to patch timers, nextTick and promises is documented here: https://github.com/nodejs/tracing-wg/blob/master/docs/AsyncWrap/README.md#things-you-might-not-expect

The reason behind https://github.com/AndreasMadsen/async-hook/blob/master/index.js#L21-L27 is just to prevent async-hook from showing up in peoples stack trace.

[AndreasMadsen]

I'm assuming in above statement, by "hot", you mean performance sensitive, and I'm assuming the hot path here is callback invocation path and not the add/remove path. Let me know if not the case, but I don't think storing callbacks as mentioned above would impact perf in any meaningful way. Would love to know why if not the case.

@mike-kaufman yes, it is the callback (hook) invocation. This happens a few times for every async operation in node. This is one of the performance sensitive parts: https://github.com/nodejs/node/blob/master/src/async-wrap.cc#L210L220

Also, the reason why I choice a simple array for holding callbacks in async-hook, is for a fast iteration in the hook call part, not for a fast registration. This may be a false assumption, I haven't benchmarked it and it's too theoretical for me to discuss (I lack knowledge about the v8 internal data structures).