🐛 Bug Report — Runtime APIs: AsyncLocalStorage not passed across Thenables

[andyjy]

The implementation of AsyncLocalStorage for Cloudflare workers doesn't pass the async context across thenables (unlike the same code running under Node.js).

Reproduction repo: https://github.com/andyjy/nextjs-tests/tree/als-thenables-edge

I found these comments from the implementation that indicate this was known at the time of implementation:

#283 (comment):

in analyzing this, we discovered that v8's API for setting the context currently does not work correctly with Thenables

#282 (comment):

Thenables are still a challenge here but there's a possible workaround using AsyncResource and we do expect v8 to fix that limitation in the future so I'm not that concerned.

..but I couldn't find this documented anywhere (specifically https://developers.cloudflare.com/workers/runtime-apis/nodejs/asynclocalstorage/)

Two issues really:

1. Documenting this behaviour difference / omission vs. Node.js.
   (I am aware Workers is explicitly not 100% compatible with Node's implementation, but this still struck me as useful to highlight - it took me considerable time to track down this issue as the cause of my code breaking when porting it from Node to Cloudflare, despite searching for all the AsyncLocalStorage-related keywords I could think of. Hopefully creating this issue helps someone else in future.)
   In particular @jasnell it would be awesome if you are able to expand on the comment "there's a possible workaround using AsyncResource" in Simplify and refine async context tracking #282 (comment).

2. Tracking support for AsyncLocalStorage working across thenables in the future - either via a workerd implementation, or awaiting the relevant upstream support to be introduced in V8 as noted in the comment above.

For context, my real-world example where this broke was using the Prisma ORM, specifically with Query Extensions: prisma/prisma#20104

Thanks!

[kentonv]

FWIW, there are actually a number of problems with using thenables in workerd. In general, API functions that expect a Promise will not accept a non-Promise thenable. Agreed this should probably be documented somewhere.

I've personally never seen thenables in the wild, and I believe this is the first time I've heard someone ask about them. Do you actually use thenables? If so, can you tell us a bit about that? We have assumed so far that they're so rarely used that it's not worth trying to implement support.

[andyjy]

Do you actually use thenables? If so, can you tell us a bit about that?

Sure! Not in my userland code, but indirectly via the Prisma ORM.

Prisma uses a thenable within their library around each async query, to defer execution of each query until then() is called as part of how they implement their API for batch transactions.

I thus hit this issue when porting my code to run via Vercel's Edge Runtime: prisma/prisma#20104

So I don't have a need for passing thenables to Workers APIs in general - just the interplay with AsyncLocalStorage as in the original issue description.

I did identify a workaround that I forgot to mention in my original post - (patching Prisma) to call AsyncLocalStorage.bind() on the callback function passed to the thenable successfully "pins" the async context from the time the thenable is instantiated. This does alter the behaviour to prevent changing to a new async context in between the instantiation of the thenable and it's eventual deferred execution via call of then(). However for me at least, that doesn't break anything for my purposes.

[kentonv]

Yeah, it should always be possible to change the thenable implementation itself to manually propagate ALS.

[irvinebroque]

@andyjy — thanks for the detailed description and repro. This is really quality feedback.

We'll update our docs (cloudflare/cloudflare-docs#9909) to clarify, and chat a bit with folks at Prisma to see what we could do to make this easier and avoid having to patch the Prisma library.

Can you share what led you down this path of trying to use Prisma <> AsyncLocalStorage together?

[andyjy]

Can you share what led you down this path of trying to use Prisma <> AsyncLocalStorage together?

Sure - I'm using AsyncLocalStorage to set request context metadata (user_id, organization_id, workspace_id, etc) within my API route handlers, and use that to implement Row-Level security (with Postgres) within a Prisma Client extension that reads the context and ensures each query is wrapped within a transaction that starts with calling a Postgres function to set the context.

Prisma's Row-Level Security example demonstrates an approach using dependency injection; I went with an implementation using AsyncLocalStorage instead of dependency injection as a different architectural choice (in particular with a simpler internal API requiring less boilerplate involving parameters passed around between controllers).

For further context about Row-Level Security with Prisma - including some other folk using AsyncLocalStorage:
prisma/prisma#12735
prisma/prisma#5128
https://dev.to/moofoo/nestjspostgresprisma-multi-tenancy-using-nestjs-prisma-nestjs-cls-and-prisma-client-extensions-ok7

We'll [..] chat a bit with folks at Prisma to see what we could do to make this easier and avoid having to patch the Prisma library

Wonderful 🙏 - I shared above but repeating in case you missed it, the issue open with Prisma is prisma/prisma#20104 and example PR open is prisma/prisma#20213

[jasnell]

The limitation with thenables is known. There's an open PR to v8 to fix the async context propagation for thenables there whicn we're waiting to pick up once it makes it's way through that process. In the meantime, I would recommend using the AsyncLocalStorage snapshot API to capture and propagate the context as necessary within the thenable itself.

[andyjy]

There's an open PR to v8 to fix the async context propagation for thenables there whicn we're waiting to pick up once it makes it's way through that process.

Thanks @jasnell - any chance there's a link you can share for where the status of this open PR for V8 can be tracked? (I dug around but not familiar with the V8 source code setup and couldn't find where to look). Cheers!

/cc: @millsp from Prisma for context - the update to V8 mentioned would (hopefully) make prisma/prisma#20213 no longer required.

[jasnell]

@Qard may have more details on status.

[Qard]

The change to fix that in V8 is here: https://chromium-review.googlesource.com/c/v8/v8/+/4674242

That should hopefully land soon. Can't comment on when cloudflare would backport that to their runtime. It's in a working state already, just need V8 folks to approve it. They may want me to write some proper tests for it though. The existing ContinuationPreservedEmbedderData API apparently doesn't have any tests. 🤔

[andyjy]

Should be fixed by #1665 (?) 🙏

(duplicate of #1513)

[kentonv]

Yes, this is fixed.