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.

[jridgewell]

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

Yes, it's just this one case that is weird.

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?

Oh yah, that's even worse. You never know when a unhandled promise would be created, and you can't do any to figure out which unhandled handler should be called.

[Flarna]

If I summarize above the proposed solution would be like this:

• capture/propagate context for the registered handler at the time then/catch is called (is often the same as promise create, e.g. in case of await of an async function call)
• capture/propagate context at the time reject is called and use it for unhandled reject handler. This should not override the context captured above

Are we sure that this is also possible in all sorts of promise chaining?

[benjamingr]

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.

Can you reproduce the behavior you don't like without using the promise constructor?

[jasnell]

@benjamingr:

Can you reproduce the behavior you don't like without using the promise constructor?

The case is pretty specific to the deferred promise pattern using the promise constructor, calling reject() from a different context, and using the unhandled rejection handler.

[dharesign]

For me, unhandled promise rejections are coding bugs. Someone somewhere forgot to add a rejection handler.

The most useful information when trying to debug an unhandled promise rejection is where the promise passed through before being unhandled. This is why async stack traces you get with async/await are great. The place the rejection originated really is inconsequential.

What do people actually do in their unhandledRejection handlers? Here's an albeit wonky example that would be a use-case for 123, not 321:

const http = require('http');

const db = require('my-db-library');

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

process.on('unhandledRejection', () => {
  const store = als.getStore();
  store.res.writeHead(500);
  store.res.end();
});

const dbConnectionP = db.connect();
  // returns a promise, rejected if the connection can't be established

const server = http.createServer((req, res) => {
  als.run({ res }, async () => {
    const dbConnection = await dbConnectionP;
    const response = dbConnection.process(req);
    res.writeHead(200);
    res.end(response);
  });
});
server.listen(80);

[jasnell]

@dharesign that case works as is in either case because the rejection is still happening within the scope the context created by the one als.run(...). Where things get a bit tricky is specifically in the case of a deferred reject that is called from a different context. This could be the case, for example, in an API like ReadableStream in which a deferred promise is created when the stream or reader is created but rejected later when a particular error occurs in the stream processing. Which context would be important in that case? The context for when the stream was created (which might have been before any als.run(...) was entered, or the context for when the rejection happened? I'd argue it's the latter in this case.

Even if we did switch the model to yield 321 by default, it would still be possible to produce 123 as a result if that's what you actually want. One would simply need to wrap the deferred reject function using AsyncResource.bind(reject) when the Promise is created so that no matter when it is called it propagates that context. The reverse, however, is not possible. In the current implementation, it is not possible to yield 321 at all since the unhandled rejection handler always enters the context captured when the promise was created.

[dharesign]

@dharesign that case works as is in either case because the rejection is still happening within the scope the context created by the one als.run(...).

It's not is it? The rejection is from within the db.connect() call which isn't in an als.run(...).

Can you give an example for ReadableStream? Rather than the context where the reject happens, isn't it the context where you consume the stream that's more important? I don't know enough about Node's Stream API to know where and when Promises would be created and rejected.

let read = fs.createReadStream('in.txt') will create a deferred Promise?

If I do pipeline(read, fs.createWriteStream('out.txt')), will pipeline internally .then() on read's deferred Promise?

Where would a rejection then originate?

[jridgewell]

It's not is it? The rejection is from within the db.connect() call which isn't in an als.run(...).

It will run correctly. Any rejection which is scoped inside any nested call from a als.run() inherits the context of that run. The only case that will change with this is if als.run(...) returns a deferred promise:

const als = new AsyncLocalStorage();

process.on('unhandledRejection', () => {
  assert.equal(als.getStore(), undefined);
});

const deferred = als.run(123, () => {
  const deferred = {};
  deferred.promise = new Promise((_, rej) => {
    deferred.reject = rej;
  });
  return deferred;
});

// The reject escaped the run
deferred.reject();

// 1. deferred.promise has not been handled
// 2. reject escaped ctx
// 3. unhandledRejection will see undefined.

Importantly, if you were to chain of that deferred inside a ALS context and have those promises be unhandled, those promises will have the correct context:

const als = new AsyncLocalStorage();

process.on('unhandledRejection', () => {
  assert.equal(als.getStore(), 123);
});

const deferred = als.run(123, () => {
  const deferred = {};
  deferred.promise = new Promise((_, rej) => {
    deferred.reject = rej;
  });

  // Create the unhandled promise here.
  // The .then() guarantees that the unhandled's internal reject will
  // inherit the 123 context.
  const unhandled = deferred.promise.then();

  return deferred;
});

// The reject escaped the run
deferred.reject();

// 1. deferred.promise was been handled
// 2. unhandled called `deferred.promsie.then()`, capturing 123 context
// 3. reject escaped ctx
// 4. unhandled becomes rejected using its restored 123 context
// 5. unhandledRejection will see 123.

[Flarna]

@jridgewell Your first sample actually gets 123 in the unhandledRejection handler with node 19.4.0 because the promise picks up the context at creation time.

With the proposed change it would receive undefined as reject runs with no store active.

[mhofman]

For me, unhandled promise rejections are coding bugs.

Off-topic, but this is really only a bug if the promise doesn't get handled before it gets dropped on the floor. IMO Node is way too aggressive in reporting these, and should only report and fail for unhandled rejected promises when the promise is collected or at program exit.

[jridgewell]

Your first sample actually gets 123 in the unhandledRejection handler with node 19.4.0 because the promise picks up the context at creation time.

With the proposed change it would receive undefined as reject runs with no store active.

Correct, I was trying to explain the difference if we accepted the proposal. Case 1 is the only difference, and Case 2 will continue to work the same as it does now. The only way to trigger the difference is to:

1. Create an unhandled promise
2. Return the unhandled promise's reject from the const reject = als.run(…) (or nested in a deferred object, etc)
3. Call that reject in a another ALS context (or the root context)

If the reject is not returned by als.run(…), then there is no difference between current and proposed semantics.

[Qard]

The AsyncResource.bind(resolve) example is interesting as it actually corresponds fairly directly to what AsyncResource was created for in the first place which is to bind a function to a context different from where it will run. This is mainly used for situations like connection pooling where the point a function is given to an API is different from where it is used. In Promises the places where the behaviour would be strange are basically all due to the resolve or reject not being called directly in the executor. The resolve path probably is in fact the most "correct" path, though changing that behaviour may produce some very unexpected results for users. There's a lot of code out there currently that would probably need AsyncResource.bind(...) calls added.

[dharesign]

It's not is it? The rejection is from within the db.connect() call which isn't in an als.run(...).

It will run correctly. Any rejection which is scoped inside any nested call from a als.run() inherits the context of that run. The only case that will change with this is if als.run(...) returns a deferred promise:

Code Snippet

const als = new AsyncLocalStorage();

process.on('unhandledRejection', () => {
  assert.equal(als.getStore(), undefined);
});

const deferred = als.run(123, () => {
  const deferred = {};
  deferred.promise = new Promise((_, rej) => {
    deferred.reject = rej;
  });
  return deferred;
});

// The reject escaped the run
deferred.reject();

// 1. deferred.promise has not been handled
// 2. reject escaped ctx
// 3. unhandledRejection will see undefined.

Importantly, if you were to chain of that deferred inside a ALS context and have those promises be unhandled, those promises will have the correct context:

Code Snippet

const als = new AsyncLocalStorage();

process.on('unhandledRejection', () => {
  assert.equal(als.getStore(), 123);
});

const deferred = als.run(123, () => {
  const deferred = {};
  deferred.promise = new Promise((_, rej) => {
    deferred.reject = rej;
  });

  // Create the unhandled promise here.
  // The .then() guarantees that the unhandled's internal reject will
  // inherit the 123 context.
  const unhandled = deferred.promise.then();

  return deferred;
});

// The reject escaped the run
deferred.reject();

// 1. deferred.promise was been handled
// 2. unhandled called `deferred.promsie.then()`, capturing 123 context
// 3. reject escaped ctx
// 4. unhandled becomes rejected using its restored 123 context
// 5. unhandledRejection will see 123.

I see what you're saying. In the case you have an escaped reject that you invoke outside of any als.run(...), I would expect to get the context of the created promise. .then(...) is really just creating a new promise and assuming the resolution status.

const als = new AsyncLocalStorage();
const makeDeferred = () => {
  const deferred = {};
  deferred.promise = new Promise((_, rej) => {
    deferred.reject = rej;
  });
  return deferred;
};

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

const deferred1 = als.run(123, makeDeferred);
const deferred2 = als.run(123, makeDeferred);
const deferred3 = als.run(123, makeDeferred);

const follow2 = als.run(234, () => deferred2.then(() => {}));
const follow3 = als.run(234, () => deferred3.then(() => {}));

als.run(345, () => follow3.then(() => {}));

als.run(456, () => {
  deferred3.reject();  // prints 345
  deferred2.reject();  // prints 234
  deferred1.reject();  // prints ???
});

I think 123 is the most logical choice. I don't think it really make sense for deferred1.reject() to cause 456 to be printed if deferred2.reject() and deferred3.reject() don't.

[Flarna]

I don't see the point why resolution/reject location is the "more correct" propagation source.

consider a simple web server using a database, reusing the connection like this:

const http = require("http");
const db = require("my-fancy-db");

const dbCon = db.connect(dbSettings); // returns a promise

http.createServer((req, res) => {
  als.run(myRequestContext, () => {
    db.then(() => {
      // I would expect myRequestContext here not undefined inherited from db.connect
    }).catch((e) => {
      // I would expect myRequestContext here not undefined inherited from db.connect
    });
  });
});

In case of unhandledRejection this might be different because there is no myRequestContext therefore a fallback to undefined (or whatever was active at db.connect`) sounds ok.

If you adapt above sample to create the db connection lazy on first request the context of first request would be kept forever.

[dharesign]

Both of those would be myRequestContext because you used .then() and .catch().

[mcollina]

The current behavior is correct.

Let's imagine a case where we store the request identifier in AsyncLocalStorage.
if somebody calls reject() outside of the current chain, the unhandledRejection handler would not have that data, making it impossible for the user to track down what request created the asynchronous operation that rejected.

[jasnell]

if somebody calls reject() outside of the current chain, the unhandledRejection handler would not have that data, making it impossible for the user to track down what request created the asynchronous operation that rejected.

This is what makes this a tricky case. Presumably the user needs to have both. In some deferred promise flows, it's going to be more important to propagate the context associated with the task resolution. In other cases, it's going to be more important to propagate the context associated with the task creation. In the overwhelming majority of cases you're going to get exactly what you want here. In the deferred promise resolution case, however, if what you really want is to propagate the task creation context, we always have the bind() option.

let resolve, reject;
const p = new Promise((a,b) => {
  resolve = AsyncResource.bind(a);
  reject = AsyncResource.bind(b);
});

With the current behavior, I do not have the flexibility to capture the context at task resolution, which is a problem. I think we should support both and the current model forces us into only one.

[Qard]

I mostly agree, but promises are not easy to modify so we'd want to do some validation that all the cases are reasonable. Perhaps we should write a doc enumerating possibly problematic promise patterns and identifying how they would behave in that context.

For example:

const storedPromise = asyncThing()

async function something() {
  await storedPromise
  // What will the store contain here?
}

storage.run('foo', something)

[jasnell]

Perhaps we should write a doc enumerating possibly problematic promise patterns

Good idea.

[Flarna]

if what you really want is to propagate the task creation context, we always have the bind() option.

The bind option effects all AsyncLocalStorage instances. If one tool requires reject/resolve path and another tool requires the init (or .then time) this is not really a solution.

I think a better solution is to add a configuration option to AsyncLocalStorage to let the user decide which sort of propagation path it wants.

This strongly remembers me to discussions if trigger_id should be used to propagate context of current async_id.

[jasnell]

I think a better solution is to add a configuration option to AsyncLocalStorage to let the user decide which sort of propagation path it wants.

I'd hesititate to add new API surface area to cover a single edge case, especially since that would likely need to carry over into the AsyncContext tc-39 proposal, but the limitation is well noted.

To illustrate the issue, if we have two separate ALS instances,

const als1 = new AsyncLocalStorage();
const als2 = new AsyncLocalStorage();

function deferredPromiseWithBind() {
	let resolve, reject;
	const p = new Promise((a,b) => {
	  resolve = AsyncResource.bind(a);
	  reject = AsyncResource.bind(b);
	});
}

const { promise, reject } = als1.run(123, () => {
  return als2.run(321, () => deferredPromiseWithBind());
});

addEventListener('unhandledrejection', () = {
  console.log(als1.getStore()); // prints 123
  console.log(als2.getStore()); // prints 321
});

als1.run('abc', () => reject());

Personally I consider this an acceptable limitation in that it's easy to reason about. I don't want to have to think about different AsyncLocalStorage instances have different propagation semantics depending on how they are created.

[Flarna]

Personally I consider this an acceptable limitation in that it's easy to reason about. I don't want to have to think about different AsyncLocalStorage instances have different propagation semantics depending on how they are created.

If both variants are needed within a single application then we need either an option to AsyncLocalStorage or maybe some other thing which does the different sort of propagation. But using an option to configure the exact behavior sounds more natural to me.

I agree that in above code this looks strange but in a real world app the two store instance would never met. One is internal to module foo and the other to module bar and they use it for independent concerns.

[jasnell]

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

[Qard]

The path wanted will vary by use case, but also could vary by the code. For example one may want promises from the resolve path but event emitters from the place handlers are added. Thinking about it, what could be reasonable is to have configuration per-source. Something like:

const store = new AsyncLocalStorage({
  bindPromiseOnInit: true,
  bindEmitterOnInit: true
})

The nice thing with following the resolve/reject path is that because it can be manually patched back to the other path via a bind(...) method we gain a natural upgrade path to directly replace the manual binds with the configuration. We could implement this initially with just the one path and consider adding the configurability later.

[Flarna]

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.

[jridgewell]

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.

[Qard]

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.

[jridgewell]

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.

[AndreasMadsen]

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.

[jasnell]

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

[Qard]

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.

[mhofman]

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.

[mcollina]

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.

[jasnell]

@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());
}));