async_hooks: fix async/await context loss in AsyncLocalStorage

[Qard]

This fixes the thenables issue discussed in #22360 at the AsyncLocalStorage level.

I think it may also be possible to fix it at a lower level without necessarily needing to make changes to the PromiseHook stuff in V8. That could maybe allow it to work in async_hooks too, but I think that'll take somewhat more work to figure out. I can always undo this later if I figure out the lower-level fix. 🤔

A more in-depth explanation of the problem can be found here: https://gist.github.com/Qard/faad53ba2368db54c95828365751d7bc

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[Qard]

cc @nodejs/async_hooks @nodejs/diagnostics

[puzpuzpuz]

Seems to be too specific workaround to me to have it in the core. It may have some performance impact too, yet it should be small and that's not the main concern here.

@Qard did you check whether the problem is really present in thenables are integrated with AsyncResource, like bluebird?

[Qard]

Yes, the problem exists in bluebird too. Every userland implementation of promises will have the same problem because it doesn't matter how the userland implementation itself works, it matters how the native promise facilities to upgrade a thenable to a native promise for the await works.

The problem is outlined here:

const cls = new AsyncLocalStorage()
const data = { foo: 'bar' }

function thenable() {
  return {
    then(cb) {
      assert.strictEqual(data, cls.getStore()) // fail
      cb()
    }
  }
}

cls.run(data, async () => {
  assert.strictEqual(data, cls.getStore()) // pass
  await thenable()
  assert.strictEqual(data, cls.getStore()) // pass
})

So basically, there is a deadzone within the thenable itself between when the then method is called and when the promise actually resolves, yielding the value back out through the await. The linear context within the async function remains intact, but the causality of the internals behind the thenable is lost.

Ideally this should be fixed at a lower level, and I'm working on that. This is just a possible fix which I know works at this level.

[Qard]

What do you think about a configuration passed to the AsyncLocalStorage constructor to turn this on manually? Would that deal with your concerns?

[puzpuzpuz]

Yes, the problem exists in bluebird too. Every userland implementation of promises will have the same problem because it doesn't matter how the userland implementation itself works, it matters how the native promise facilities to upgrade a thenable to a native promise for the await works.

The problem is outlined here:
...

The problem shown in your snippet can be easily fixed in the thenable implementation (thus, in user-land) by integrating with AsyncResource (so, bluebird should work as expected).

I've slightly modified your snippet to make it a valid test and added the missing AsyncResource part:

'use strict';
require('../common');
const assert = require('assert');
const { AsyncLocalStorage, AsyncResource } = require('async_hooks');

const asyncLocalStorage = new AsyncLocalStorage();
const data = { foo: 'bar' };

function thenable() {
  const res = new AsyncResource('Thenable'); // here is the main part
  return {
    then(cb) {
      res.runInAsyncScope(() => { // this call is also important
        assert.strictEqual(data, asyncLocalStorage.getStore());
        cb();
      });
    }
  };
}

async function main() {
  await asyncLocalStorage.run(data, async () => {
    assert.strictEqual(data, asyncLocalStorage.getStore());
    await thenable();
    assert.strictEqual(data, asyncLocalStorage.getStore());
  });
}

main();

[Qard]

That works in the case where you have a factory function which you can put the construction of the AsyncResource in, but the reality is that the vast majority of thenable issues derive from fluid or reusable objects. The most common case being database query builder libraries like knex, which use the call to then to join the current state of the built query and kick off the connection to the database. There is no point in the chain at which it knows itself to be "done" building until the then is called, and that is too late to create the AsyncResource. This means that almost all existing cases of AsyncResource used in the wild today do not work with async/await.

[Qard]

For context, here's the then method implementation in knex: https://github.com/knex/knex/blob/e37aeaa31c8ef9c1b07d2e4d3ec6607e557d800d/lib/interface.js#L18-L33

That is merged into a bunch of other classes, none of which have sufficient awareness of the point at which the runner would be invoked to be able to know when to create an AsyncResource instance or how to store it to retrieve later, when the then method actually gets called.

[Qard]

Another side note: a fix of some sort for this needs to exist in core for domains to be stable. Domains need to be able to track through internals to be able to adequately contain the execution and internal errors.

[mcollina]

I’m -1 to this fix. This is going to slow everything down because of the use use of before and after hooks.

[Qard]

I'm thinking a config option like trackAsyncAwait which would switch between two hook sets so it only does the before/after if a user of AsyncLocalStorage has explicitly requested it. What do you think?

[mcollina]

I'm not sure. I think we should investigate if there is another possible fix first, this API is extremely nice and losing so much perf would not be good for the ecosystem.

[Qard]

Yep, I'm trying to figure out a lower-level fix. I just made this as a possible higher-level solution for now, until we can come up with something better. Agreed it's not great though, needing the extra before/after hooks.

[mcollina]

Considering that this problem is not new of AL, I don't think this should be rushed in.

[Qard]

Yep, that's fine. Just putting something up that I can iterate on. If the possible lower-level fix is what it needs to be to land, so be it. :)

[puzpuzpuz]

That works in the case where you have a factory function which you can put the construction of the AsyncResource in, but the reality is that the vast majority of thenable issues derive from fluid or reusable objects.

If a custom thenable can return different results on multiple calls to then, it's broken from the Promise/A+ spec perspective. We shouldn't try to fix such libraries in the core.

In case of knex, which seems to follow the spec and return the same object, an AsyncResource could be created at the beginning of the fluent chain and when then is called a runInAsyncScope function could be called, just like it's done in #33189 (comment). As that comment shows, it's enough to integrate a custom thenable with async/await + AsyncLocalStorage.

This means that almost all existing cases of AsyncResource used in the wild today do not work with async/await.

Could you provide examples of situations when a well-behaved custom thenable can't be integrated with async_hooks by using AsyncResource? If there are any and this PR fixes them, they should be covered with tests.

In general, I don't see how this PR fixes any real problems with custom thenables. IMO we already have the proper solutions, namely, AsyncResource and native promises. Both of these options will integrate user-land module with async_hooks-based CLS implementations, when used properly. This statement also applies to non-async/await promise syntax.

If that's required, I can also go into more details of potential context loss cases in user-land modules, that I'm aware of.

[Qard]

The problem though is that the before/after will link back to where the new AsyncResource occurs though, and there's not a clear "correct" place to do that in many cases.

With knex, the knex object will be shared across all queries so it probably shouldn't be in the construction of that.

You can do knex('table') to start operating on a certain table, so you might think to put it there but you could also do knex.select().from('table').

You can also reuse intermediate results:

const baseQuery = knex('accounts as a1')
  .leftJoin('accounts as a2', function() {
    this.on('a1.email', '<>', 'a2.email');
  })
  .select(['a1.email', 'a2.email'])

const data = await baseQuery.where(knex.raw('a1.id = 1'))
const data = await baseQuery.where(knex.raw('a1.id = 2'))

The baseQuery could be create anywhere, including outside of the request, which would mean creating the resource somewhere in the would link to entirely the wrong place.

So where would the AsyncResource get constructed for those queries? If you create a fresh AsyncResource for every single call most would never be used and you'd be polluting the async_hooks event stream with tons of inits that never actually have a before/after.

The cognitive burden of expecting the ecosystem to fix our context loss problems for us instead of fixing it properly ourselves can get very high. We need to fix this properly or we will continue to have modules like this which break context, forcing us to do even worse hacks to attempt to restore it.

I'm not arguing for this fix specifically but that we absolutely need some fix for this.

[puzpuzpuz]

So where would the AsyncResource get constructed for those queries? If you create a fresh AsyncResource for every single call most would never be used and you'd be polluting the async_hooks event stream with tons of inits that never actually have a before/after.

Looks like where serves as a terminal operation in your snippet. If that's the case, that should be the point when AsyncResource is created.

If it's not the case and knex doesn't have the the concept of terminal operations (which sounds like a bad design to me), they could at least introduce a list of potentially terminal operations and only create AsyncResource there. This will reduce number of redundant allocations significantly. In case if that's still a concern (TBH I don't think it is), this behavior can be enabled via a library config option.

A side note. The concept of reusable thenables seems to be a broken design to me. Custom thenables should behave like native Promises, i.e. once resolved they always return the same result. In case of knex it seems that the query object serves as a thenable and it can be mutated. What's even worse, each mutation can return a different result in calls of then/catch. This can be easily fixed, say, by introducing toPromise method for query object.

The cognitive burden of expecting the ecosystem to fix our context loss problems for us instead of fixing it properly ourselves can get very high. We need to fix this properly or we will continue to have modules like this which break context, forcing us to do even worse hacks to attempt to restore it.

I'm not arguing for this fix specifically but that we absolutely need some fix for this.

Considering your arguments, I still don't think we should fix anything, as we do provide Embedder API to do the job. Instead we should educate library maintainers how to integrate correctly with AsyncLocalStorage by either using AsyncResource (or native promises - for instance, this can be done by suggesting users to promisify callback API of their libraries).

[Flarna]

Considering your arguments, I still don't think we should fix anything, as we do provide Embedder API to do the job. Instead we should educate library maintainers how to integrate correctly with AsyncLocalStorage by either using AsyncResource

My thoughts on this:
Please note that async-hooks including the embedder API is still experimental. As long as this doesn't change it's not that likely that module authors will pick up AsyncResource more frequently as noone likes to depend on something which can change APIs even in minor versions.

If we don't fix this in core we may end up in having userland cls modules including such a fix which then "just work for users" but AsyncLocalStorage does not.

[puzpuzpuz]

@Flarna

Please note that async-hooks including the embedder API is still experimental. As long as this doesn't change it's not that likely that module authors will pick up AsyncResource more frequently as noone likes to depend on something which can change APIs even in minor versions.

AsyncLocalStorage is experimental as well, so library authors may be fine with using AsyncResource under a certain config option, like it's done in bluebird.

However, these is a much simpler way to correctly integrate with AsyncLocalStorage without using AsyncResource. I'm speaking about native promises which represent a stable API. In case of the majority of apps/libraries, this should be the right way to have ALS working properly.

If we don't fix this in core we may end up in having userland cls modules including such a fix which then "just work for users" but AsyncLocalStorage does not.

The problem with this workaround is that we don't have any user reports that describe real world bugs. Thus, this workaround seems to be synthetic and claims to fix the problem for libraries that have weird thenable API, like knex.

On the other hand, client libraries for databases and data stores that deal with TCP sockets trigger operation result callbacks on socket.on('data'). Thus, in such libraries the callback is called on a totally different context and AsyncResource is a must have for proper ALS integration here (unless the library doesn't use native promises). I'm not sure if knex belongs to this group of libraries, but most likely it is. So, that's why the fix provided in the PR seems to be synthetic to me.

[targos]

@Qard I do not understand which usage of knex is broken with our current implementation.

I tried something like this in a request handler:

// async/await
queryBuilder.where(something).andWhere(somethingElse)
console.log(cls.getStore());
const result = await queryBuilder;
console.log(cls.getStore());

// then
queryBuilder.where(something).andWhere(somethingElse)
console.log(cls.getStore());
queryBuilder.then((result) => {
  console.log(cls.getStore());
});

In all cases, cls.getStore() returns the correct value so it doesn't look like context is lost.

[Qard]

@targos The problem is not continuing context, it is internal context. If you need to get the store from somewhere within the execution of the queryBuilder thenable execution, which is the standard case for every APM product, there will be no context. This is also specific to async/await, because the calling of the then method occurs within a microtask tick with no async_hooks context. Your code calls the then method directly in user code, so it would have a context to connect to there.

As shown in #33189 (comment), the context is available before and after a thenable, but not within it.

@puzpuzpuz We don't see reports here because the issue only surfaces in APM products, the actual users of this API. As someone that has worked in APM for half a decade building three different APM products, I am reporting this. I have seen this issue reported on my agents literally hundreds of times and have had to do horrible and fragile hacks to attempt to get around this.

[puzpuzpuz]

@puzpuzpuz We don't see reports here because the issue only surfaces in APM products, the actual users of this API. As someone that has worked in APM for half a decade building three different APM products, I am reporting this. I have seen this issue reported on my agents literally hundreds of times and have had to do horrible and fragile hacks to attempt to get around this.

Could you check my considerations above? Your fix may not resolve the problem in many cases.

Also, wouldn't it better to submit patches against such libraries instead of trying to place a (partial) workaround in the core? I don't think there are a lot of popular libraries that belong to the discussed group.

[Qard]

As I already explained: some libraries, especially knex, have no clear place to put a fix. I have patched a few libraries with AsyncResource before where it was clear how to do so. However there is a clear pattern of module authors trying to use thenables in more advanced ways like this which is making it harder and harder for APMs to keep a stable context. This problem will only get bigger with time and trying to just tell users to code a certain way is not going to help much when most aren't listening, and the few that are would often miss the point of supporting a feature they don't use directly. I've had AsyncResource PRs rejected before because the module owner didn't feel they needed the feature themself and they didn't trust an API that is marked experimental.

As for your comment about it being a "partial" fix--I don't see how that is the case. It certainly doesn't 100% eliminate all cases where AsyncResource is needed--queuing patterns will always be a thing--but it does ensure that specific boundary is always linked correctly, allowing the subsequent chain of internal code to be patch appropriately in the context it needs rather than needing to pile on some very sketchy and non-performant hacks in userland to try and re-establish that context before doing anything.

[puzpuzpuz]

As I already explained: some libraries, especially knex, have no clear place to put a fix. I have patched a few libraries with AsyncResource before where it was clear how to do so.

If I would be working on APM and would be considering monkey patching knex to make it AsyncLocalStorage friendly, I'd try to do the following.

I would allocate an AsyncResource in query object constructor and store it as a property. There would be also a thenCalled boolean property (false by default), which would be set to true on then invocation. Then I would monkey patch all query methods (or at least potentially terminal ones, like where, andWhere, etc), so they would have the following check:

if (this.thenCalled) {
  this.asyncResource = new AsyncResource('KnexQuery');
  this.thenCalled = false;
}

Of course, the callback in then method has to be wrapped with runInAsyncScope. It also should do this.thenCalled = true.

The idea here is that the AsyncResource should be refreshed only after then was called and the query is reused.

Hopefully, you'll find this helpful.

As for your comment about it being a "partial" fix--I don't see how that is the case. It certainly doesn't 100% eliminate all cases where AsyncResource is needed--queuing patterns will always be a thing

It's not necessary about queuing. See #33189 (comment)

[Qard]

That's the comment I'm referring to. The issue you mention is separate and easily patchable if the context is available somewhere else in the chain. Without a fix for the thenables issue however, there might not be a context accessible at the appropriate place to patch that.

[puzpuzpuz]

Do you see any problems with the monkey patch logic described in #33189 (comment) ? That logic could be submitted as a patch for knex (protected by a config option), if the authors are totally against migrating to native promises.

[Qard]

Lots of potential timing issues. What if you do two queries at the same time with Promise.all(...)? How would you pass two separate resources into the two separate microtasks without a way to pass data alongside an await? You can't store the reference on the object because both microtasks will run in the same tick and there won't be anywhere in-between that the reference can be updated to match the microtask. There's also not really clear "terminal" operations, as you put it, so you'd still have to make a bunch of AsyncResource instances that never reach the before/after events.

Also, using native promises would require a breaking change which wouldn't help all the people stuck on the older versions. APM vendors need to support whatever module set the customer has in their app. We can't just tell them they can't use our product unless they update all their modules. Many dependencies they will not have control of, and actually quite often APM gets used by companies that built something once, years ago, and do not intend to update the code until absolutely necessary. I know of at least one very major company still using Node.js 0.8 in production because it just works, and they will probably never update it until either the server catches fire or they go out of business. 🙄

[Qard]

I put it behind an option. I'll continue trying to figure out a lower-level fix.

[puzpuzpuz]

You're framing this as broken thenable implementations, but that's not the case at all. This is completely valid code according to the spec for thenables which we are handling incorrectly in Node.js core.

Promise/A+ spec assumes that thenables expose a spec-compliant then method. In case of knex multiple calls to then may lead to different fulfilled/rejected values. I may be wrong here, but this doesn't seem to be a spec-compliant behavior.

This sort of push back is exactly why it took over half a decade to get a CLS implementation in Node.js core.

I'm afraid that such generalization is not applicable here. I wouldn't be having concerns if the following would not hold:

• The fix works only when async/await syntax is in use.
• The fix is aimed against some libraries with specific thenable behavior.
• A niche user code pattern is assumed (async_hooks: fix async/await context loss in AsyncLocalStorage #33189 (comment)).
• The fix has certain performance implications.

Once again, if the fix is hidden under a constructor option and there is a consensus in the work group, I'm willing to change my vote to neutral.

[Flarna]

Not blocking comment: I'm not a big fan of user facing options which could be even named disableProblems...
Anyhow, some doc update is needed. And maybe we should use a more clear name as async await is tracked in general.

[mcollina]

My (strong) -1 to this approach still stand. Adding an option is not enough because the problematic bits are the before and after hooks.

I would also note that the name of the option is misleading, as async/await works fine. The problems are thenables.

---

I would recommend an alternative approach, something like a wrap() method that a user can wrap a problematic thenable with to fix this problem instead of providing a blanket solution that will inevitably slow everything down.

[legendecas]

@mcollina I would recommend an alternative approach, something like a wrap() method that a user can wrap a problematic thenable with to fix this problem instead of providing a blanket solution that will inevitably slow everything down.

AsyncResource does the wrap now. Anyway, @Qard have stated their concerns on manual wrapping delaying the growth of async_hooks usage. If the approach here nodejs/diagnostics#376 (comment) will be a better alternative to this patch for the problem surfaced?

[benjamingr]

Is there some way to slow down only custom thenables without impacting native promises?

[Qard]

@mcollina The before and after hooks only exist when someone has explicitly asked for them now though, so that shouldn't be so much of an issue. If there are no AsyncLocalStorage objects with the trackAsyncAwait option then it will use to original init-only version.

For the option name, I'm open to whatever name people think is clearest. :)

As for the manual wrap idea, that's already basically what AsyncResource does. It's meant to just be the backup plan though. It leaks concerns for context propagation into userland, and most places its needed are places that the developer of the code had no reason to think about async_hooks. Due to the potential for thenables to exist anywhere, any place that anyone awaits anything would potentially need to be patched or the context would be lost. This makes module interop in an async/await-based world extremely fragile.

One of the main reasons the "thenables" concept exists is to support ecosystem promise implementations like bluebird. Without support for it in async_hooks to correctly assign ids to thenables, all these libraries are needing to patch themselves to not break the world. This is problematic, especially given that this means that anyone writing and publishing a Node.js module to do anything async needs to be aware of this or they might publish something that breaks the world. That adds an extra cognitive burden to the focus of every single developer that wants to write stuff with Node.js.

In its current state, async_hooks is fundamentally broken because within any awaited thenable it will have zero as a trigger id and an empty resource stack, which should be impossible. Thenables are not kicked off within a context we control so it starts off with no async_hooks context whatsoever and then could potentially create an entire orphaned chain of async behaviour beneath it.

Because of how ubiquitous thenables are in userland--almost every single database or cache module uses them--async_hooks very often gets broken in user apps. And it's not just a small context loss here or there. In most cases, they lose 90% of the async graph that APM products attempt to capture. I've worked with four different APM vendors and all have had by far the largest user complaint was context loss resulting in little to know data in the APM dashboard. I've seen literally hundreds, if not thousands, of users complaining about it on the various APM products I've worked on. And those are just the ones vocal enough to bother contacting support about it or opening an issue. I'd bet many more just give up and uninstall without reporting.

I get that AsyncResource can fix a lot of cases, but there's also hundreds of thousands of modules on npm. There's no possible way APM vendors could review even a fraction of those for context loss issues and open pull requests. At past APM companies I spent 90% of my time tracking down context loss issues for whatever user was having issues that day, and the majority of times it was some obscure module with only a couple thousand downloads that they just happened to be using for one thing in their system, but the presence broke the context everywhere because it just happened to be in the path between two other more common modules deep in the internals of some dependency somewhere. 😬

[mhdawson]

@Qard do you have the simplest example of using a thenable that breaks the context, and then the additional code you'd need to fix the problem with AsyncResource? That might clarify/highlight the burden we'd be asking package maintainers to take on.

[devsnek]

Is there anything I can do on the V8 side to help fix this?

[Qard]

I'm working on a gist explaining exactly how to reproduce the problem, what the implications are, and abstractly what needs to be solved to "correctly" handle it. I'll post it here when I'm done.

[Qard]

Here's the gist explaining the problem in more detail: https://gist.github.com/Qard/faad53ba2368db54c95828365751d7bc

If you have any more questions, I'll try to expand the examples further.

[mhdawson]

@Qard, that is a great write up! One thing I thing that would be worth adding is what the developer would have to do in the first example to resolve the problem using AsyncResource. You follow up with the wrong things people do, but I think showing what we'd expect people to do for every thenable type method without your fix would be useful.

[Qard]

Good suggestion @mhdawson. I've expanded that a bit, rearranged the doc some and elaborated in some other areas too. I'm also going to add something later elaborating on the technical details of exactly where in the Node.js core code this issue is happening and the technical reasons why it's difficult to solve.

[puzpuzpuz]

@Qard thanks for writing the problem description gist.

As the current implementation doesn't change default behavior and performance of ALS, I'm changing my review result to neutral. IMO if this fix gives ALS some users among APM tools, that would be great.

I also think that the option name could be changed to something more obvious (say, forceThenablesPropagation or something like that?) and it should be properly documented.

[benjamingr]

@Qard it might also be worth it to add a link to the (excellent) gist in the PR description

[Qard]

I'm a bit on the fence about if it should be documented at all. It'd be a step beyond just "experimental" more toward "you probably shouldn't use this unless you really know what you are doing and why" 🤔

Also, my hope is that even if this does manage to land that we can figure out a lower-level fix and therefore not even need it later. This PR is basically an experiment and demonstration that the issue can be fixed without needing to hack up V8, and also a possible temporary fix, if we decide we want it enough.

[puzpuzpuz]

I'm a bit on the fence about if it should be documented at all. It'd be a step beyond just "experimental" more toward "you probably shouldn't use this unless you really know what you are doing and why" 🤔

If this PR gets landed, it should include documentation changes. The "you probably shouldn't use this unless you really know what you are doing and why" can be included in the doc section. Otherwise, this option stays invisible for users who might be interested in the workaround (APM devs), which kills the idea.

As for a low-level fix, it would be great to have it instead of this workaround.

[Flarna]

For an end users building applications which use the one or the other framework with some plugin,... it's mostly impossible to find out if someone uses thenables.
Tool and module developers have no possibility to control in which environment they are used so they have to assume the worst setup.

I agree with @Qard that adding and option and try to document it understandable is most likely not the best choice.

If an option is added I would vote for having the variant supporting thenables enabled on default and allow users to disable it and warn in the docs about context loss.

As a side note: If we go the way to allow users to trade functionality vs performance we should even allow users to disable the whole (expensive) Promise tracking as not all applications use promises.

[mcollina]

I would like to make my objection clear: if this was pointed out when AsyncLocalStorage landed, I would have -1 that. This should be fixed properly and not with a band-aid that has extreme performance cost. In other term, I think a Node.js without AsyncLocalStorage is a better one than one with a slow AsyncLocalStorage.

I would be ok in adding an experimental warning to AsyncLocalStorage that goes along the line of "This API is going to change a lot in the future, do not use it yet", which is in the exact opposite direction of the reason why AsyncLocalStorage was added.

[BridgeAR]

Should this be closed in favor of #33560?

[Qard]

Yep. Closing.