🐛 Bug Report — Runtime APIs: The AsyncContext of AsynLocalStorages gets lost in custom thenables

[dario-piotrowicz]

If I define a custom thenable and try to access the store of an AsyncLocalStorage in the then method, the store seems to be undefined although it shouldn't be.

Basically:

import { AsyncLocalStorage } from 'node:async_hooks';

const als = new AsyncLocalStorage();

export default {
  async fetch() {
    return als.run({ }, async () => {
      const thenable = {
        then(onfulfilled) {
          // ...
          als.getStore(); // <- yields undefined ❌
        },
      };
       // ...
    });
  },
};

minimal reproduction

[jasnell]

Ah, yes, custom thenables do not currently automatically propagate the async context. You'll need to use AsyncLocalContext.bind(...) or AsyncLocalContext.snapshot(...) with the thenable to capture the context.

const thenable = {
  then: AsyncLocalStorage.bind(function (onfulfilled) {
    als.getStore();
  })
};

[dario-piotrowicz]

@jasnell I see but I assume that this can be improved/fixed? is propagating the async context for thenables something potentially problematic/complex?

As you can see in cloudflare/next-on-pages#499 the problem is that libraries can use thenables so application authors could be unable use the bind/snapshot workaround 🥲

[jasnell]

It requires a fix in v8 to get working. We can patch v8 to get it working but I've been waiting on a larger change coming for the implementation of the standardized AsyncContext API. I was hoping that would come faster than it has but it's looking like there's no ETA yet. I can work up the patch and we can float it until v8 receives the other updates.

[dario-piotrowicz]

@jasnell yeah that would be awesome since I have seen a few developers hitting this issue 😄

Hopefully it's not too too much work/complex to implement such temporary patch? 🤞

[paramaggarwal]

Any suggested workaround meanwhile? I am unable to use Prisma on CloudflareWorkers currently.

[jasnell]

The immediate workaround is to use AsyncLocalStorage.snapshot(...) ...

For example... (this doesn't use a thenable but the idea should be clear)...

class Foo {
  #runInAsyncScope = AsyncLocalStorage.snapshot();

  get() { return this.#runInAsyncScope(() => asyncLocalStorage.getStore()); }
}

Essentially, your custom thenable would use AsyncLocalStorage.snapshot() to capture the relevant context and enter it manually when the thenable is invoked.

[dario-piotrowicz]

@jasnell I assume that the V8 change hasn't landed yet? 😓

Regarding the workaround, as I mentioned here: cloudflare/next-on-pages#499 (comment) I think that unfortunately the workaround is a bit impractical when thenables come from libraries.

Since people keep hitting this issue I would be curious to know if there's an ETA for the V8 change, alternatively as I mentioned in my next-on-pages comment maybe we could there do some hacks and try to patch all thenables present in the code, but before we consider doing that I'd be curious about the effort required in workerd to patch this and the brittleness of the solution since, as I mentioned, a next-on-pages hack would be:

• brittle
• complex
• costly (it would likely significantly increase build time)
• fix this problem only for apps using next-on-pages

So I'm wondering, if we want to fix this, where it would make more sense to do so (I'm definitely leaning towards workerd but as I mentioned I don't really know what the fix there implies)

[jasnell]

... where it would make more sense to do so

No ETA yet on the v8 change. I'll look at it today. The change definitely needs to be made in v8. We don't have the necessary visibility in to the necessary bits within v8 to apply a reasonable workaround in workerd.

[jasnell]

... The change definitely needs to be made in v8...

To clarify, when I say this, I mean that the limitation here is within v8 itself and to fix it requires a patch to v8. We can float a patch that addresses this but ultimately, long term, it's something that v8 needs to address. My plan currently is to float a patch and I have one in testing internally. Assuming that looks good I'll be able to open a PR here.

[paramaggarwal]

Oh I see. Thanks for looking so deeply into this. Do you think there is long-term merit in using JavascriptCore instead of V8? It would reduce resource utilisation on Cloudflare Edge Network right?

[jasnell]

... long-term merit in using JavascriptCore instead of V8 ...

Not necessarily. While we've always had a Side Quest in mind of making workers somewhat agnostic of the js runtime the reality is that v8 is a hard dependency. It's not clear at all that switching to something like JSC would yield enough of a realized benefit to justify the development effort it would take. This issue around the async context propagation, on its own, certainly would not be enough justification.

[jasnell]

Quick update... I've got the v8 patch put together and internal testing is showing that it's good. I want to get some review before I open the PR that adds the patch here. But, progress!

@Qard ... just a heads up that I was able to use your older patch as a roadmap here. When I open the PR here, the patch should contain everything you need to move forward to upstream the updated patch in v8 itself.

[Qard]

Nice! That was quick work. 🎉

[rakibatomnixima]

waiting for the patch, thanks for your hard work!

[jellohouse]

Waiting for the patch as well - Hope it gets fixed soon!
Is there an ETA?

[kentonv]

Fixed by #1665 which was merged yesterday. Likely will go to production some time next week.

[jasnell]

Going to go ahead and close this issue now since the fix has landed. As Kenton mentioned, if all goes well that'll end up in production likely by next week.

[janpio]

We @prisma would appreciate a further comment here when this has been rolled out to production, so we can confirm and then remove documentation for the current blocking problem. Thanks.

[kentonv]

This is now in production.