AsyncLocalStorage and deferred promises

@bmeck

Take the following case:

const { AsyncLocalStorage } = require('async_hooks');
const als = new AsyncLocalStorage();

process.on('unhandledRejection', () => {
  console.log(als.getStore());
});

let reject;
als.run(123, () => { new Promise((a,b) =>reject = b); });
als.run(321, () => reject('boom'));

What value should the console.log(als.getStore()) print to the console?

Currently, it prints 123 because the async context is captured as associated with the Promise at the moment it is created (in the kInit event).

I'd argue, however, that it should print 321 -- or, more concretely, that it should capture the async context at the moment the promise is resolved, not at the moment the promise is created, but the current behavior could also be correct.

/cc @nodejs/async_hooks @bmeck @bengl @mcollina

It seems easier for me to wrap my head around the context being the one at promise creation: this is the originative event that creates the async task 🤔

My intuition was 123 but probably that is infected by me thinking too much about current internals recently... If a rejection is generated outside of any als.run call, or by Node Core itself instead of other JS code, then would you want it to log undefined or 123? This is sort of a case where I thought stashing the async context in kInit is useful.

I would argue that for promises it is more correct to capture the context at the moment reject() or resolve() is called. For instance,

class Foo {
  constructor() {
    this.deferred = createDeferredPromise();
  }

  void resolve() { this.deferred.resolve(); }
  void reject() { this.deferred.resolve(); }
}

const foo = new Foo();

als.run(123, () => foo.resolve());

If I specifically wanted to capture the context when the Foo was created, then what I should do is extend AsyncResource and use runInAsyncScope inside both foo.resolve() and foo.reject() ... otherwise, this case should be similar to EventEmitter.emit(...).

It's really hard to think about this stuff abstractly. How about in terms of use cases? Has anyone come across a case where they need to look at AsyncLocalStorage from inside the unhandledRejection event?

(Bloomberg definitely has an analogous internal case where we currently depend on restoring the AsyncLocalStorage from kInit in unhandledRejection, but this is a code path based on our custom V8 embedding, not Node.js. Probably our usage pattern is not representative of a typical Node app, and maybe we could adopt this AsyncResource pattern; I'm not sure.)

Main use case for ALS till now was context propagation. e.g. consider following

function myWebRequestHandler(req, res) {
  const span = startSpan("myWebRequest");
  als.run(span, () => {
    await callSomeDb(query);
    await callSomeDb(other query);
    const activeSpan = als.getStore(); // getting anything else then active span here woudl be strange. 
  });
}

The resolve of above promise happens in some unrelated code, maybe even some native callback.

I think the correct place to capture the context would be when promise.then() is called. In above code this is the same as when the promise is created.

123 in unhandledReject handler seems to be also correct to me as it referes to the context where the failing operation happens belongs to.

It's really hard to think about this stuff abstractly. How about in terms of use cases? Has anyone come across a case where they need to look at AsyncLocalStorage from inside the unhandledRejection event?

good point, in my cases, I want to associate an unhandled rejection with the http request the server was working on. Or at least whatever network transaction the rejection is part of.

@jasnell

@jasnell I see your point and now I am not certain of anything anymore 😅

I think the correct place to capture the context would be when promise.then() is called. In above code this is the same as when the promise is created.

Note that this isn't quite correct. Yes, the context is captured when promise.then() is called, but for async/await, it's when the await happens:

let resolve;
const deferred = new Promise((r) => {
  resolve = r;
});

const p = als.run(123, async () => {
  await deferred;
  console.log("123", als.getStore());
});
als.run(321, async () => {
  await p;
  console.log("321", als.getStore());
});

resolve("abc");

If there is no then or await, then I think it's reasonable to assume the context at the time resolve/reject is called. It also means that Promises will be cheaper to implement, because they don't need to carry their initialization context only for unhandledrejection.

I think we all agree that, in the case where a .then() or await exists, that's the place to capture the context for the callback arguments to then or the statement immediately after the await. The question is just, what to do if no then/await occurs? (This includes the unhandled rejection case.) The current solution in Node.js is to say that the capture occurs at the previous then/await, which is where the Promise was created. The alternative, to capture where the resolve/reject is called, is a lot more non-local IMO.

In general it makes sense to propagate a context from promise subscription being added (then/await) to execution of reactions.

In this case we don't have either. However I would argue than more often than not, the context that subscribes to the promise is the same as the context that creates the promise, but I'm not sure it justifies capturing and using that context.

Propagation of context is also done with the explicit intent to disconnect the resolving context from the reaction execution context. I really don't see why the rejecting context should be the one used in the unhandled event.

My question is, why is the context in which the event handler was added not the one used?

What should the following example produce?

const { AsyncLocalStorage } = require('async_hooks');
const als = new AsyncLocalStorage();

als.run(0, () => {
  process.on('unhandledRejection', () => {
    console.log(als.getStore());
  });
});

const panic = als.run(123, () =>Promise.reject(Error('panic')));
panic.catch(() => {});

als.run(321, () => Promise.resolve(panic));

The more I go through this, the more I'm convinced that 321 is the right answer and that the current behavior implemented by Node.js is wrong.

My question is, why is the context in which the event handler was added not the one used?
What should the following example produce?

That is an option, but it means you must also remember to unregister your handler or you're going to get multiple logs:

function handleRequest(req, res) {
  als.run({}, () => {
    const unhandled = () => {
	  console.log(als.getStore());
	};
	process.on('unhandledRejection', unhandled);

    try {
      // work work work…
    } finally {
      process.off('unhandledRejection', unhandled);
    }
  });
}

But your example actually won't log anything (the promise is handled, and Promsie.resolve(rejected) === rejected, it won't create a new promise). If we did new Promise(r => r(panic)), then it'd log 321 by the internal .then() registration.

But your example actually won't log anything (the promise is handled, and Promsie.resolve(rejected) === rejected, it won't create a new promise). If we did new Promise(r => r(panic), then it'd log 321 by the internal .then() registration.

Oops, ugh promise adoption is tricky.

Ok so this seem to be really limited to the deferred use case and unhandled rejections?

That is an option, but it means you must also remember to unregister your handler

I don't think that'd work if the work is async since there is no way to know when that async flow is done, right?

Ok so this seem to be really limited to the deferred use case and unhandled rejections?

Yes, very much so.

Keep in mind that the 'unhandledrejection' event listener itself can be bound to a specific context if you really want that.

It's of no help to overwrite the context. All participants here want async context propagation. Some want to find the resolver/rejected others the creator. And both should fit into a single application.

Thinking about it, what could be reasonable is to have configuration per-source. Something like:
const store = new AsyncLocalStorage({
  bindPromiseOnInit: true,
  bindEmitterOnInit: true
})

I would suggest we consider what the future will look like when we have AsyncContext, and the likelihood any of these changes will pass the committee. I've gotten extremely positive committee feedback so far, and I expect that we'll have something similar to the current AsyncContext API in JS. The more host-customisability we allow on the API, the more pushback this is going to get in the committee. The committee may never accept these changes.

In a future where we have a simple AsyncContext implementation, code is naturally going to move from using Node's AsyncLocalStorage to the standardized one. I want us to think of a single set of semantics that we can use for both implementations, and minimize these differences.

If Node requires the current AsyncLocalStorage semantics, then we'll figure out how to work it into the spec. If not, then then let's change Node to use AsyncContext's semantics. But let's not fall into designing two separate interfaces for the core functionality.

Well, that's where the bind/wrap function comes in. Having a friendly configuration pattern allows simplifying use of these different patterns in the future if we so choose, but they're really just simplifications of the manual bind so we can just continue using those too if pushback would be too much. I'm also not advocating we include that in the initial proposal. What I'm saying is that if we go the resolve/reject path we can use bind to map that back to what Node.js might expect, but we can't do the reverse so we should take that into consideration.

Having a friendly configuration pattern allows simplifying use of these different patterns in the future if we so choose, but they're really just simplifications of the manual bind so we can just continue using those too if pushback would be too much.

This is going to hit the same performance constraints I commented in #46374 (comment). In short, I want us to design an API that does not impact the performance of running code. Adding this configuration requires that we change the wrap algorithm from O(1) to O(n) (n for all AsyncContexts currently allocated). Adding even the potential for configuration will really hurt all code that uses promsies, every time a continuation is created.

What I'm saying is that if we go the resolve/reject path we can use bind to map that back to what Node.js might expect, but we can't do the reverse so we should take that into consideration.

Agreed.

In my humble opinion, the premise of this discussion is invalid. Neither New Promise, reject, or resolve is an actual async operation, they don't involve any microtask. New Promise should never have created an AsyncResource to begin with, that was a design mistake.

As I suggested in nodejs/diagnostics#389, .then() should be what creates the AsyncResource since it creates a microtask. Using promise_hooks, which is now a public API, you can do extra work to capture the context you think is correct. Although, my personal opinion would still be that capturing context for AsyncLocalStorage at New Promise, reject, or resolve is incorrect.

@AndreasMadsen

@AndreasMadsen ... first off, great to "see" you! It's been a while since we've talked!

Neither New Promise, reject, or resolve is an actual async operation, they don't involve any microtask. New Promise should never have created an AsyncResource to begin with, that was a design mistake... .then() should be what creates the AsyncResource since it creates a microtask.

I think we're all generally in agreement on this particular point! However: with an unhandled rejection there explicitly is no then we can use to propagate the context. This case is really about that particular issue.

What I'm suggesting in this conversation is not that resolve() or reject() capture the context, but that, internally, the act of scheduling the 'unhandledrejection' event to be fired is itself an async resource that should capture the current context (which just so happens to be the context that is current when reject() is called). It's a subtle detail but important for the semantics here.

This is going to hit the same performance constraints I commented in #46374 (comment). In short, I want us to design an API that does not impact the performance of running code. Adding this configuration requires that we change the wrap algorithm from O(1) to O(n) (n for all AsyncContexts currently allocated). Adding even the potential for configuration will really hurt all code that uses promsies, every time a continuation is created.

That's not exactly true. If we keep the configuration variety low you can bucket same configurations together and just pick which context bucket to copy at each viable connection point. You see a promise construction and you copy the promise construction flowing bucket. You see a promise resolve and you copy the promise resolve flowing bucket. The configuration would not change for the lifetime of that storage so it's easily optimizable into behaviour buckets.

What I'm suggesting in this conversation is not that resolve() or reject() capture the context, but that, internally, the act of scheduling the 'unhandledrejection' event to be fired is itself an async resource that should capture the current context (which just so happens to be the context that is current when reject() is called). It's a subtle detail but important for the semantics here.

I think this is the clearest way to frame the problem as it's exactly what is happening. The spec has a host hook to inform the host when a promise becomes rejected and is unhandled, and when it becomes handled afterwards. These hooks are "called" synchronously but obviously the host doesn't trigger a program visible event from them immediately as that would be very noisy (and re-entrant), and instead waits for either the end of the current promise job, or the draining of the promise job queue depending on the host implementation (I'd still argue the host should wait for the finalization of the promise and simply trigger an uncaught error event, but that's another story).

In any case, the host schedules a new job that will execute later when the engine synchronously informs it of an unhandled rejection. Capturing the current async context at the time of rejection is the natural and logical behavior.

As I suggested in nodejs/diagnostics#389, .then() should be what creates the AsyncResource since it creates a microtask. Using promise_hooks, which is now a public API, you can do extra work to capture the context you think is correct. Although, my personal opinion would still be that capturing context for AsyncLocalStorage at New Promise, reject, or resolve is incorrect.

+1. This seems significantly better and would solve quite a lot of the problems.

@AndreasMadsen

@AndreasMadsen :

my personal opinion would still be that capturing context for AsyncLocalStorage at New Promise, reject, or resolve is incorrect.
@mcollina :
+1. This seems significantly better and would solve quite a lot of the problems.

We're in agreement here. Just different ways of saying the same thing.

Capturing the context at New Promise is not actually necessary or helpful in the typical case.

For instance, consider the following cases:

// This example creates two separate promises.
const als = new AsyncLocalStorage();
const p = als.run(123, () => new Promise((res) => {
  // This runs in a synchronous scope. The promise does not need
  // to capture the async scope here.

  // This schedules async activity, the setTimeout captures the async
  // scope...
  setTimeout(() => {
    console.log(als.getStore());
    res();
  }, 1000);

  // Using the init promise hook to capture the async context here,
  // especially the way we do it currently in Node.js where context
  // is propagated for every new promise, is unnecessary because
  // it will *never* be used
}));

// The then here captures the async context on the continuation.
// The promise *itself* does not *need* the async context attached
// because it is only relevant to the continuation.

als.run(321, () => p.then(() => {
  console.log(als.getStore());
}));

Or this, which might be clearer:

// This example creates two separate promises.
const als = new AsyncLocalStorage();

// Capturing the context at this New Promise is obviously pointless.
// It won't ever be used and is a very wasteful operation the way we have
// things currently implemented. There's just simply no reason to
// capture the context for new Promise.
const p = als.run(123, () => Promise.resolve());

als.run(321, () => p.then(() => {
  console.log(als.getStore());
}));

This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.

Navigation Menu

Repository navigation

AsyncLocalStorage and deferred promises #46262

24 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

AsyncLocalStorage and deferred promises #46262

Description

Activity

vdeturckheim commented on Jan 18, 2023

littledan commented on Jan 18, 2023

jasnell commented on Jan 18, 2023

littledan commented on Jan 18, 2023

Flarna commented on Jan 18, 2023

vdeturckheim commented on Jan 18, 2023

vdeturckheim commented on Jan 18, 2023

jridgewell commented on Jan 18, 2023

littledan commented on Jan 18, 2023

mhofman commented on Jan 18, 2023

jasnell commented on Jan 19, 2023

jridgewell commented on Jan 19, 2023

mhofman commented on Jan 19, 2023

jasnell commented on Jan 19, 2023

24 remaining items

Flarna commented on Jan 26, 2023

jridgewell commented on Jan 26, 2023

Qard commented on Jan 26, 2023

jridgewell commented on Jan 26, 2023

AndreasMadsen commented on Jan 26, 2023

jasnell commented on Jan 26, 2023

Qard commented on Jan 26, 2023

mhofman commented on Jan 27, 2023

mcollina commented on Jan 27, 2023

jasnell commented on Jan 27, 2023

github-actions commented on Jun 22, 2026

github-actions commented on Jul 23, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions