AsyncLocalStorage and deferred promises

[jasnell]

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

[vdeturckheim]

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 🤔

[littledan]

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.

[jasnell]

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

[littledan]

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

[Flarna]

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.

[vdeturckheim]

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.

[vdeturckheim]

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

[jridgewell]

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.

[littledan]

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.

[mhofman]

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));

[jasnell]

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.

[jridgewell]

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.

[mhofman]

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?

[jasnell]

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

Yes, very much so.