01:05
<Justin Ridgewell>
I'm surprised you need to do so much work to integrate it
01:06
<Justin Ridgewell>
The spec has a hook designed for this usecase, and it seems to exist in V8: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=6837-6853;drc=dd38db94df728f36d61d5d6c943156dbe37144a4
01:07
<James M Snell>
I spotted that previously but couldn't find any detail whatsoever on what/how it's used
01:09
<James M Snell>
specifically, on the promise hook callback, for the init event, we create an opaque JS object that wraps the reference to the current async context frame. We then set that as a private field on the Promise object. What's not clear in any way (I couldn't find any documentation or examples) if Set/GetContinuationPreservedEmbedderData could be used for that
01:10
<James M Snell>
specifically, it's not clear when we would call SetContinuationPreservedEmbedderData and what effect that would have
01:14
<Justin Ridgewell>
If I'm reading this right, I would call it in Set… inside AsyncContext.run(), and call Get… inside AsyncContext.get()
01:15
<Justin Ridgewell>
01:16
<Justin Ridgewell>
Looks like Yoav might be using it in the new Chrome TaskAttribution work: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/scheduler/task_attribution_tracker_impl.cc;l=233-289;drc=71630a0e336a703e21de9ebeb98a5abf84e8c96c
01:18
<James M Snell>
Still not super clear but it's worth exploring to see if it does make it easier
01:19
<James M Snell>
one key issue is that we need the context tracking to also work with things like timers
01:19
<Justin Ridgewell>
What other times do you provide?
01:20
<James M Snell>
so long as we can preserve the context with non-v8 things and it all just works, then hopefully this could make things easier. I'll play with it
01:20
<James M Snell>
Just setTimeout and setInterval but v8's only involvement there is that we call the function when the timer expires
01:20
<Justin Ridgewell>
I think if you port https://docs.google.com/presentation/d/1yw4d0ca6v2Z2Vmrnac9E9XJFlC872LDQ4GFR17QdRzk/edit#slide=id.g18e6eaa50e1_0_192 into C++, and everywhere you access __storage__ you call Get…, and everywhere you set you call Set…, then it'd work correctly
01:20
<James M Snell>
we capture a reference to the current frame and enter it when we invoke the callback
01:22
<James M Snell>
ok, I'll play with this a bit. If we can eliminate the promise hook I'll be really quite happy
01:25
<Justin Ridgewell>
It doesn't look like anything else uses the embedder continuation data, so I'm not sure if setTimeout would propagate correctly.
01:25
<Justin Ridgewell>
It might be easy to do manually, though
13:52
<littledan>
The spec has a hook designed for this usecase, and it seems to exist in V8: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=6837-6853;drc=dd38db94df728f36d61d5d6c943156dbe37144a4
Oh wow I didn’t know about this API
13:57
<littledan>
Would be cool to see if someone can build AsyncContext as a Node native module just based on that, or if there are limitations
20:30
<Andreu Botella>
SCOPING.md has the following:
20:31
<Andreu Botella>

Only code with direct access to AsyncContext instances can modify the value, and only for code execution nested inside a new context.run().

20:32
<Andreu Botella>
Doesn't AsyncContext.wrap allow any code to modify the value of all AsyncContext instances in the callback's context?
20:33
<Justin Ridgewell>
Yes and no
20:34
<Justin Ridgewell>
The AsyncContext instance can't contain a value unless that value is placed there by someone with access to the instance
20:34
<Justin Ridgewell>
undefined (the default state of an AsyncContext) is just a "value placed there by someone with access"
20:35
<Justin Ridgewell>
So restoring the state to a prior one just places a value into the AsyncContext that was already placed there by someone with access
20:36
<Justin Ridgewell>
Mark Miller's equates this more to a closed over variable
20:37
<Justin Ridgewell>
If I have a opaque Map (that I can't directly acccess, but which can be access by AsyncContext instances) which holds the data, then all I've done is propagate my closed-over map for you to read from.
20:37
<Justin Ridgewell>
20:40
<littledan>
I guess to connect these things, the modification is well-behaved
20:40
<littledan>
yes, you definitely can restore things via wrap, that's the whole point, that is a well-behaved modification
20:41
<littledan>
so maybe this could be clarified in the wording of SCOPING.md
20:41
<Andreu Botella>
yeah, in that sense wrap doesn't introduce any dynamic scoping, so it's just a matter of wording that
20:41
<James M Snell>
Just a quick update here. I've implemented use of this api to store context for workers (https://github.com/cloudflare/workerd/pull/282).... it works well for promises in general but does have a limitation.
20:42
<James M Snell>
Specifically, I'm currently not able to completely eliminate the use of the promise hook because the v8 api does not make the context propagation available for the unhandledrejection event handlers
20:42
<Andreu Botella>
I'm not familiar with programming language theory, and there's a lot of stuff I'm missing here
20:42
<James M Snell>
I end up having to still associate the current context frame with the promise in the promise hook and entering the frame manually when emitting those events
20:43
<littledan>
Specifically, I'm currently not able to completely eliminate the use of the promise hook because the v8 api does not make the context propagation available for the unhandledrejection event handlers
yep, that doesn't surprise me... this is what you need the init hook for, right?
20:43
<littledan>
but it's great to hear if everything but the init hook works!!!
20:44
<James M Snell>
Second, I am capturing the async context when queueMicrotask() is called so that the context at the time queueMicrotask is called to schedule the callback is entered. v8 does not appear to propagate the stored context when invoking those scheduled tasks
20:44
<littledan>
I'm not familiar with programming language theory, and there's a lot of stuff I'm missing here
Andreu, I recommend you make PRs to SCOPING.md to make things clearer for you, and we can iterate on it in code review. You shouldn't have to be an expert in PL theory to read our docs.
20:44
<James M Snell>
but otherwise, the v8 api is quite helpful. Thank you for pointing it out Justin Ridgewell
20:45
<littledan>
Second, I am capturing the async context when queueMicrotask() is called so that the context at the time queueMicrotask is called to schedule the callback is entered. v8 does not appear to propagate the stored context when invoking those scheduled tasks
Sorry, could you explain this a little more?
20:45
<Andreu Botella>
well, that was a bit more about Justin Ridgewell's response
20:45
<James M Snell>
als.run(123, () => queueMicrotask(() => console.log(als.getStore())));
20:45
<littledan>
the thing is, Mark Miller's school of PL theory is really not widely understood... it is just his small community
20:46
<James M Snell>
I've implemented it such that when the scheduled microtask is run, als.getStore() returns the appropriate value
20:46
<littledan>
als.run(123, () => queueMicrotask(() => console.log(als.getStore())));
huh, I thought that would be the whole point of V8's API, to propagate over exactly that pattern... if it doesn't handle this, what does it do?
20:47
<James M Snell>
I don't know. I could be doing something wrong here but in local testing it wasn't picking up the value
20:47
<James M Snell>
I can play with it more
20:48
<littledan>
did you get it to work at all, in any case? for example, maybe it works for promises but not calls to queueMicrotask?
20:49
<Justin Ridgewell>
the thing is, Mark Miller's school of PL theory is really not widely understood... it is just his small community
Mark asked to update the proposal docs to reference his explanations, which I'm a little worried about. 😬
20:50
<littledan>
Mark asked to update the proposal docs to reference his explanations, which I'm a little worried about. 😬
reference like link to, or put everything in his terms?
20:50
<Justin Ridgewell>
They may not be integrating with the job queue correctly
20:50
<Justin Ridgewell>
The intention of the hooks is for that to work without modifications, but they're only calling it for promises currently
20:50
<Justin Ridgewell>
I'm not sure how Web APIs implement their calls
20:51
<Justin Ridgewell>
Redo the explanation in his terms
20:53
<James M Snell>
yeah, definitely doesn't appear to be working
20:53
<littledan>
Redo the explanation in his terms
Ah, yeah, I think we should decline this request. The main explanation should be intuitive; we could have a doc off to the side in their terms
20:53
<Justin Ridgewell>
Specifically, I'm currently not able to completely eliminate the use of the promise hook because the v8 api does not make the context propagation available for the unhandledrejection event handlers

Looks like that's actually possible with SetPromiseRejectCallback:

20:54
<littledan>

Looks like that's actually possible with SetPromiseRejectCallback:

Justin, that's one side (where you read the relevant asynccontext) but there's the other side separately (in the promise init hook, when you have to stash exactly that information)
20:55
<littledan>
yeah, definitely doesn't appear to be working
Yoav told me that, for web APIs, he added extra logic to save and restore the right thing when queueing [macro]tasks
20:55
<littledan>
so this is why I'm wondering if, in your testing, the save and restore at least happened for promises (it'd be really surprisng to me if it didn't work)
20:56
<James M Snell>
Will the promise reject callback be invoked while the appropriate stored continuation value is current?
20:57
<James M Snell>
if so, I can absolutely capture it then
20:57
<Andreu Botella>
littledan: You're sometimes replying to a thread on the main channel. Are you using an outdated Matrix client?
20:57
<littledan>
I'm using app.element.io... I don't see threads
20:58
<littledan>
how do I enable them?
20:58
<Andreu Botella>
oh, they're not actually enabled by default in recent Element versions, I just had manually enabled them
20:58
<Andreu Botella>
All Settings / Labs
20:59
<Justin Ridgewell>
Yes, it should be
20:59
<James M Snell>
oh! yes, quick test appears that it does work! woo!
21:00
<Justin Ridgewell>
Ah, yeah, I think we should decline this request. The main explanation should be intuitive; we could have a doc off to the side in their terms
That's exactly what I said. Lol.
21:01
<littledan>
Oh hi now I can see the thread
21:01
<James M Snell>

Yoav told me that, for web APIs, he added extra logic to save and restore the right thing when queueing [macro]tasks

Justin Ridgewell in v8 or somewhere else?

21:02
<James M Snell>
if in v8, it might be in a newer version than we're using maybe?
21:02
<Justin Ridgewell>
Hm?
21:02
<littledan>

Yoav told me that, for web APIs, he added extra logic to save and restore the right thing when queueing [macro]tasks

Justin Ridgewell in v8 or somewhere else?

This is in Chrome, not V8
21:02
<littledan>
so you won't just pick it up
21:02
<James M Snell>
ok yeah
21:03
<littledan>
Will the promise reject callback be invoked while the appropriate stored continuation value is current?
I don't expect this to work at all... I think we'd have to add something specifically to promises code to store exactly this thing
21:03
<James M Snell>
Well, it does appear to be working in local testing
21:03
<littledan>
this is why it's important that we document that unhandled rejection asynccontext is important!
21:03
<littledan>
huh really? how so?
21:03
<littledan>
Could you share your tests?
21:04
<James M Snell>
          jsg::AsyncContextFrame::Scope scope(js, jsg::AsyncContextFrame::current(js));
          auto ev = jsg::alloc<PromiseRejectionEvent>(event, kj::mv(promise), kj::mv(value));
          dispatchEventImpl(js, kj::mv(ev));

It's a bit obscure here but...

jsg::AsyncContextFrame::current() is a wrapper around code that grabs the current stored continuation data from v8::Context

21:06
<James M Snell>

In JavaScript...

const { AsyncLocalStorage } = async_hooks;

const als = new AsyncLocalStorage();

addEventListener('unhandledrejection', (event) => {
  // Here we are making sure that the async context properly travels along with
  // unhandled rejections.
  if (event.reason === 'boom' && als.getStore() !== 123) {
    throw new Error("Incorrect async store value");
  }
  if (event.reason === 'boom2' && als.getStore() !== 'ABC') {
    throw new Error("Incorrect async store value");
  }
});

addEventListener('rejectionhandled', (event) => {
  if (als.getStore() !== 'ABC') {
    throw new Error("Incorrect async store value");
  }
});

export default {
  async fetch(request) {
    // The async context is captured when on(...) is called to register
    // handlers.
    als.run(123, () => {
      Promise.reject("boom");
    });

    const p = als.run('ABC', () => {
      return Promise.reject("boom2");
    });

    await scheduler.wait(1);

    p.catch(() => {});

    return new Response("ok");
  }
}
21:06
<Justin Ridgewell>
Try one that adopts the state of a rejected promise
21:06
<Justin Ridgewell>
That'll be the tricky case
21:07
<Justin Ridgewell>
Promise.reject is a sync rejection signal, so it's very easy to capture the current run state
21:07
<James M Snell>
oh.. no, yeah, shoot
21:08
<James M Snell>
spoke too soon. That case was failing
21:10
<James M Snell>
boo ok
21:16
<Justin Ridgewell>
Curious of your test case
21:17
<Justin Ridgewell>
The spec flows work, maybe it's just a V8 impl bug
21:17
<Justin Ridgewell>
The reject is either called sync (in which case, the context is still there), or it's queued in a job callback, in which case it would have been restored before calling reject
21:18
<James M Snell>
I'll see if I can dig in a bit deeper soon
21:20
<Justin Ridgewell>
new Promise((_, rej) => rej(1));
new Promise(res => res(Promise.reject(1)));
// technically equivalent to the previous one
Promise.resolve().then(() => Promise.reject(1));
22:20
<littledan>
FYI I asked about this API in the #jsc room of WebKit Slack, and Jared Sumner responded explaining that he's implementing AsyncLocalStorage in Bun. Hopefully he takes my offer to come and hang out over here :)
22:23
<littledan>
fun fact: Promise.reject is in fact the hard case, since you actually want to wait for the microtask queue to be exhausted before considering it truly an unhandled rejection
22:23
<littledan>
the async case where the rejection happens after the responses have already been chained on--that's the easy case
22:24
<littledan>
(this was quite tricky for people inside of Bloomberg to rediscover, at least)
22:29
<James M Snell>

For the following case, what would you expect the als.getStore() value to be printed..

addEventListener('unhandledrejection', () =>{
  console.log(als.getStore());
})

let reject;
const p2 = als.run(123, () => new Promise((a,b) => reject = b));
als.run(321, () => reject());
22:30
<Justin Ridgewell>
Ahh, I hadn't considered that reject() would be called in a different context
22:32
<Justin Ridgewell>
I don't have an answer for this, it's not a usecase that we expose
22:33
<Justin Ridgewell>
All user code is guaranteed to run inside the only context we care about
22:34
<James M Snell>
not sure I grok that... in Node.js, als.getStore() in the unhandledrejection handler prints 123
22:34
<James M Snell>
because it grabs the async context reference on promise init and not promise reject
22:36
<Justin Ridgewell>
I think that's reasonable, but the other way actually allows more expressiveness
22:36
<Justin Ridgewell>
If you wanted to preserve the context for the reject(), you would export return wrap(reject)
22:37
<James M Snell>
yeah, I'm a bit torn because honestly I do think it should return 321 in this case
22:37
<Justin Ridgewell>
I wonder if node had a use case to choose init-time
22:37
<James M Snell>
I'm not entirely convinced this case was considered
22:38
<James M Snell>
I'm going to open an issue in Node.js to discuss
22:38
<Justin Ridgewell>
👍️, please link
22:42
<littledan>

For the following case, what would you expect the als.getStore() value to be printed..

addEventListener('unhandledrejection', () =>{
  console.log(als.getStore());
})

let reject;
const p2 = als.run(123, () => new Promise((a,b) => reject = b));
als.run(321, () => reject());
I would say, 123. I guess I'd predict that the V8 API that Justin linked to would log undefined.
22:42
<littledan>
This is actually important for Bloomberg's (admittedly weird) use case
22:42
<James M Snell>
https://github.com/nodejs/node/issues/46262
22:43
<Justin Ridgewell>
I think it'd log 321, no?
22:44
<Justin Ridgewell>
reject there is https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-promise-reject-functions, which immediately invokes https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-rejectpromise, which triggers the hook API
22:45
<James M Snell>
Yeah, it logs 321
22:46
<Justin Ridgewell>
You would need to store the current context until the actual unhandledrejection event is fired, but you can do that as part of the hook
22:47
<Justin Ridgewell>

https://github.com/nodejs/node/issues/46262

Nit: "promise is resolved" should say "promise is settled"

22:47
<littledan>
yeah, this is exactly the thing that I raised a week or two ago; I'm glad we're coming back to it since apparently we were talking past each other then
22:48
<littledan>
I was arguing, we should include something about the kInit storage in the spec (even though it's not visible without the unhandled rejection tracking)
22:48
<James M Snell>
My intuition is that what you really want is the context at the point either resolve() or reject() is called
22:49
<Justin Ridgewell>
I remember. I think we should specify this as part of the proposal, I'm just interested in how we can implement this for workerd given what we already have
22:49
<James M Snell>
it's trivial for me to make it work either way now.
22:49
<James M Snell>
currently I have the Promise hook in place to set the context on kInit
22:50
<James M Snell>
but I have a pending set of changes that eliminates the promise hook and captures it on the rejection handler, which is working great
22:52
<littledan>
I remember. I think we should specify this as part of the proposal, I'm just interested in how we can implement this for workerd given what we already have
what do the spec sections that you cited have to do with workerd?
22:52
<littledan>
but I have a pending set of changes that eliminates the promise hook and captures it on the rejection handler, which is working great
wait, does this work to implement either set of semantics, or just the 321 one?
22:53
<James M Snell>
it's either or, not both
22:54
<James M Snell>
my pending changes removes the promise hook
22:54
<Justin Ridgewell>
workerd uses v8, and v8 exposes these hooks for implementers.
22:55
<Justin Ridgewell>
I think we should specify a behavior instead of allowing implementers to implement on their own
22:55
<James M Snell>
For this case, I think we have to
22:55
<littledan>
will this be a publicly visible PR?
22:55
<James M Snell>
yes
22:56
<Justin Ridgewell>
https://github.com/cloudflare/workerd/pull/282
22:58
<James M Snell>

Specifically, I think by default, we should capture the context on settle not kInit. So the answer by default should be 321. If you want 123, then use wrap around reject as suggested by Justin. Or, if we are using a mechanism like Node.js' AsyncResource, we can do something like:

class Foo extends AsyncResource {
  constructor() {
    this.deferredPromise = createDeferredPromise();
  }

  resolve() { this.runInAsyncScope(() => this.deferredPromise.resolve()) }
  reject() { this.runInAsyncScope(() => this.deferredPromise.reject()) }
}
22:59
<James M Snell>
This gives us the most flexibility because it allows us either option
23:00
<littledan>
Can we have a parallel thread about the same question in legendecas's repo? I'd feel more comfortable sharing my thoughts there (since they're more related to Bloomberg's use case than Node stuff)
23:09
<Justin Ridgewell>
https://github.com/legendecas/proposal-async-context/issues/16
23:42
<Justin Ridgewell>
This is also an interesting case because unhandledrejection will be the only event API that does not preserve the context at the time of registration...
23:44
<littledan>
It would indeed be special, but Yoav found that, in DOM, some events naturally wanted to restore the context when they were registered, whereas others should not do so and leave in place the context that triggered them
23:44
<littledan>
(I don't know details, but when we get back in touch with him, I'm really excited to hear his thoughts.)
23:44
<littledan>
this is sort of a third case, to be sure
23:45
<littledan>
but I think we agree it's disagreeing somehow, whether we go with 321 or 123
23:47
<James M Snell>
Going through this, I'm more and more convinced the answer should be 321
23:48
<James M Snell>
if we're saying that the context is rightfully captured when the then() is called, or when await is called.. invoking an unhandledrejection handler falls into the equivalent category of "continuation handler"... it's just a different kind of continuation
23:53
<littledan>
I need to pull in an expert in how Bloomberg's system currently works before we make a call for the proposal in general; this whole thing might be unusable to us if the answer is 321