Repository navigation
discuss: future of AsyncWrap #7565
Description
I've been taking a look at the AsyncWrap this week and I think it's time we discuss whether it has a future in core. Things that stand out:
- Besides a 1.5 year old NodeSource blog post it's undocumented.
- It is unclear to me whether there are any users.
- There is only minimal test coverage. For example, the functionality of the
_asyncQueueproperty is untested. It's hard to determine whether AsyncWrap is really in a working state (also because of point 2); it's almost certainly broken for things like timers that don't have a 1-to-1 mapping to libuv handles. - It results in a fair amount of complexity and duplication. For example, the
node::MakeCallback()andnode::AsyncWrap::MakeCallback()functions are just different enough that it's awkward to unify them, resulting in ~200 lines of duplicated code.
For the sake of keeping core maintainable, I would like people to comment on whether AsyncWrap is something that we should keep.
cc @nodejs/ctc, @trevnorris in particular, and @AndreasMadsen
Activity
addaleax commentedon Jul 6, 2016
Forgive me if I’m mistaken, but isn’t nodejs/node-eps#18 basically the answer to this?
bnoordhuis commentedon Jul 6, 2016
I didn't want to hijack the EPS. It focuses on what the API should look like while I'm more concerned with whether the complexity of the existing implementation is a worthwhile trade-off.
(I do have some reservations about the AsyncWrap API - IMO, it's more complex than it needs to be - but I haven't had time yet to fully articulate my thoughts. That's something for the other issue, though.)
AndreasMadsen commentedon Jul 6, 2016
There is an up-to-date documentation at https://github.com/nodejs/diagnostics/tree/master/tracing/AsyncWrap This is not in the official node.js documentation because it is an unofficial API. This allows us to make API changes as we discover problem.
I use it for my long stack trace module trace, which is used by plenty. If I make a mistake I will usually get an issue report within 12 hours :)
I also made a wrapper (async-hook) for AsyncWrap, it has a few dependents: https://www.npmjs.com/browse/depended/async-hook
I agree, the testing is minimal. The initial testing was almost none existing, but we continue to improve it. As for
_asyncQueueit should in my opinion just be removed, as it doesn't serve any real purpose.In the documentation there is a list of known issues,
timersis among those. I also maintain an issue overview such we don't forget: nodejs/diagnostics#29 The EP nodejs/node-eps#18 should also address these issues.My AsyncWrap wrapper solves most issues. I think these issues are fairly easy to work around, which is why they haven't been prioritized.
Good point, I haven't spent much time in this area.
A problem that we are currently facing is that
node::MakeCallback()which is used by native addons doesn't invoke AsyncWrap (unless ugly hacks are used). This breaks the async tree/handle context making it impossible to for example make a long-stack-trace or have a continuation-local-storage setup. Last I heard @trevnorris had some fantastic solution to this, I suspect that would require unifying them.trevnorris commentedon Jul 6, 2016
Now that's not completely fair. The tests in
test/parallel/test-async-wrap-*are focused on areas that are intended to be stable going forward. While things like_asyncQueueare simply a wart that will be removed (mention more below).This was working in the initial implementation when
AsyncListenerlanded, but the decision to remove the JS API portion just before a major release left the API in an awkward state. Because fixing timers will require touching JS I decided to not to fix it until the native API stabilized more (e.g. now).I'm writing up additional API for the EP that should solve this. Basically a public API that leverages
node::AsyncWrap::MakeCallback()sonode::MakeCallback()can disappear. Also solving the issue of users not being able to track their own API calls, and removing_asyncQueuefrom existence.I would love to hear your feedback. The API now is a result of my best effort at reducing the large number of use cases and features that have been requested since its initial conception.
jasnell commentedon Jul 6, 2016
The code duplication and complexity issues are certainly valid... and keeping it around as an undocumented experimental thing indefinitely is not good. The decision was made on the CTC call today to move forward with @trevnorris' EPS. What I would suggest is this: Let's evaluate this in October. If significant progress is not made on making AsyncWrap a stable, documented, full supported feature by then, or if any complexity added by the updated implementation that @trevnorris is working on is too much to swallow, we look to explore other options. In the meantime, let's be patient and continue to let things play out.
bnoordhuis commentedon Jul 11, 2016
Sorry for the delay, the notification got buried in my inbox. Okay, I'm fine with seeing how things play out and revisit in a few months. Closing issue.
@AndreasMadsen @trevnorris Thanks for the feedback.
Fishrock123 commentedon Jul 15, 2016
Do keep in mind we'll still need to work towards something if we want to keep domains deprecated, and, there are still people in need of better tracing/CLS solutions than domains.
I'm sure it's invasive, any solution will be. My understanding was that it was less so than domains, or at least better collected to be more understandable.
I'm sure, at any rate that Zones would be far more invasive, at least wrapping every API endpoint. Not to mention perf costs there without opt-in or opt-out.
I'd also like to see us continue to pursue this for usage reasons, I've been meaning to put some work into dprof. I feel like tools like that could greatly help people understand their applications, and also better learn Node.js.
(Sorry for the late reply!)
AndreasMadsen commentedon Jul 23, 2016
@Fishrock123 regarding CLS did you see nodejs/diagnostics#57?
PS: thanks for the kind works about dprof, I will look forward to your input.
acarstoiu commentedon Jan 31, 2017
I know this issue is closed, but I couldn't find any conclusion on this matter, nor an answer in #7040. Is
async_wrapdead? 'Cause dying it is.jasnell commentedon Jan 31, 2017
Far from it. The async_hooks capability is under active development with an open PR in the main branch. The goal is to land that PR as soon as possible.
acarstoiu commentedon Jan 31, 2017
Thanks for answering. Is there a future "official" documentation?
joyeecheung commentedon Jan 31, 2017
Latest updates on async hooks can be found in diagnostics WG meeting notes (also, the latest one)
joyeecheung commentedon Jan 31, 2017
Or if you prefer reading commits...perhaps subscribe this PR
acarstoiu commentedon Jan 31, 2017
For other interested people: https://github.com/nodejs/node-eps/blob/523e9aa48a304701a5ab492103b1b4b3873184db/006-asynchooks-api.md