discuss: future of AsyncWrap

[bnoordhuis]

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:

1. Besides a 1.5 year old NodeSource blog post it's undocumented.
2. It is unclear to me whether there are any users.
3. There is only minimal test coverage. For example, the functionality of the _asyncQueue property 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.
4. It results in a fair amount of complexity and duplication. For example, the node::MakeCallback() and node::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

[addaleax]

Forgive me if I’m mistaken, but isn’t nodejs/node-eps#18 basically the answer to this?

[bnoordhuis]

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]

1. Besides a 1.5 year old NodeSource blog post it's undocumented.

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.

1. It is unclear to me whether there are any users.

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

1. There is only minimal test coverage. For example, the functionality of the _asyncQueue property is untested. It's hard to determine whether AsyncWrap is really in a working state (also because of point 2);

I agree, the testing is minimal. The initial testing was almost none existing, but we continue to improve it. As for _asyncQueue it should in my opinion just be removed, as it doesn't serve any real purpose.

it's almost certainly broken for things like timers that don't have a 1-to-1 mapping to libuv handles.

In the documentation there is a list of known issues, timers is 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.

It results in a fair amount of complexity and duplication. For example, the node::MakeCallback() and node::AsyncWrap::MakeCallback() functions are just different enough that it's awkward to unify them, resulting in ~200 lines of duplicated code.

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]

There is only minimal test coverage. For example, the functionality of the _asyncQueue property is untested.

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 _asyncQueue are simply a wart that will be removed (mention more below).

it's almost certainly broken for things like timers that don't have a 1-to-1 mapping to libuv handles.

This was working in the initial implementation when AsyncListener landed, 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).

It results in a fair amount of complexity and duplication. For example, the node::MakeCallback() and node::AsyncWrap::MakeCallback() functions are just different enough that it's awkward to unify them, resulting in ~200 lines of duplicated code.

I'm writing up additional API for the EP that should solve this. Basically a public API that leverages node::AsyncWrap::MakeCallback() so node::MakeCallback() can disappear. Also solving the issue of users not being able to track their own API calls, and removing _asyncQueue from existence.

(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.)

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]

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]

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]

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]

@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]

I know this issue is closed, but I couldn't find any conclusion on this matter, nor an answer in #7040. Is async_wrap dead? 'Cause dying it is.

[jasnell]

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]

Thanks for answering. Is there a future "official" documentation?