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 |
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> |
|
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 |
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 |
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 |
20:45 | <Andreu Botella> | well, that was a bit more about Justin Ridgewell's response |
20:45 | <James M Snell> |
|
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> |
|
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 |
20:50 | <littledan> | Mark asked to update the proposal docs to reference his explanations, which I'm a little worried about. 😬 |
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 |
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 |
20:54 | <littledan> |
|
20:55 | <littledan> | yeah, definitely doesn't appear to be working |
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 |
21:01 | <littledan> | Oh hi now I can see the thread |
21:01 | <James M Snell> |
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> |
|
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? |
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> |
It's a bit obscure here but...
|
21:06 | <James M Snell> | In JavaScript...
|
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> |
|
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
|
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> |
|
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> | 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 |
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 |
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
|
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 |