-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Native prototype of ECMAScript AsyncContext
proposal using existing public v8::Context
methods
#2
base: main
Are you sure you want to change the base?
Conversation
This branch of my https://github.com/benjamn/rusty_v8 fork are based on v8 version tag v0.60.1.
In principle, I would like this declaration to be available as esnext.asynccontext, but I haven't found a good way to import those types by default (important for initial developer experience, IMO), whereas all the globals in the deno.shared_globals.d.ts module are always available globally.
AsyncContext
using only existing public v8::Context
methodsAsyncContext
using only existing public v8::Context
methods
AsyncContext
using only existing public v8::Context
methodsAsyncContext
using existing public v8::Context
methods
Warning: this does not yet build the current contents of the repository before running the tests. It just uses the image provided as the first argument (benjamn/deno:async-context by default) with whatever version is currently published to Docker Hub.
baca00f
to
5039ba6
Compare
AsyncContext
using existing public v8::Context
methodsAsyncContext
proposal using existing public v8::Context
methods
describe("async via promises", () => { | ||
// This wrapping is no longer needed, since the native implementation of | ||
// AsyncContext hooks into native PromiseReactionJob creation and execution. | ||
// | ||
// beforeEach(() => { | ||
// Promise.prototype.then = then; | ||
// }); | ||
// afterEach(() => { | ||
// Promise.prototype.then = nativeThen; | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I meant by avoiding "Promise.prototype.then
wrapping."
AsyncContext
proposal using existing public v8::Context
methodsAsyncContext
proposal using existing public v8::Context
methods
Adapted from my example code in #2.
This will require a rebuild of benjamn/deno:async-context-builder and benjamn/deno:async-context Docker images: cd docker docker build . --no-cache \ -f ./Dockerfile.async-context.builder \ -t benjamn/deno:async-context-builder docker build . --no-cache \ -f ./Dockerfile.async-context \ -t benjamn/deno:async-context
That failing test was using the old |
it("works with thenables", async () => { | ||
const ctx = new AsyncContext(); | ||
const queue: string[] = []; | ||
const thenable = { | ||
then(onRes: (result: string) => any) { | ||
const message = "thenable: " + ctx.get(); | ||
queue.push(message); | ||
return Promise.resolve(message).then(onRes); | ||
}, | ||
}; | ||
|
||
return ctx.run("running", async () => { | ||
assert.strictEqual(ctx.get(), "running"); | ||
|
||
assert.strictEqual( | ||
await new Promise<any>(res => res(thenable)), | ||
"thenable: running", | ||
); | ||
|
||
await Promise.resolve(thenable).then(t => t).then(async result => { | ||
assert.strictEqual(result, "thenable: running"); | ||
return ctx.run("inner", async () => { | ||
assert.strictEqual(ctx.get(), "inner"); | ||
assert.strictEqual(await thenable, "thenable: inner"); | ||
assert.strictEqual(ctx.get(), "inner"); | ||
return "👋 from inner ctx.run"; | ||
}); | ||
}).then(innerResult => { | ||
assert.strictEqual(ctx.get(), "running"); | ||
assert.strictEqual(innerResult, "👋 from inner ctx.run"); | ||
}); | ||
|
||
assert.strictEqual(ctx.get(), "running"); | ||
|
||
return thenable; | ||
|
||
}).then(thenableResult => { | ||
assert.strictEqual(thenableResult, "thenable: running"); | ||
assert.strictEqual(ctx.get(), void 0); | ||
assert.deepStrictEqual(queue, [ | ||
"thenable: running", | ||
"thenable: running", | ||
"thenable: inner", | ||
"thenable: running", | ||
]); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new test demonstrates AsyncContext
is now preserved even when using Promise
-like thenable objects (any object with a function-valued then
method). This behavior did not work with my previous Docker image benjamn/deno:async-context
, but it works now with benjamn/deno:async-thenables
.
Technically, adding support for thenables boiled down to making PromiseResolveThenableJobTask
behave more like PromiseResolveReactionJob
, in benjamn/deno-v8@d72a3dd, my fork of Deno's fork of V8.
This change was inspired by two comments from @jridgewell:
- AsyncLocalStorage without Async Hooks nodejs/node#46265 (comment)
- AsyncLocalStorage without Async Hooks nodejs/node#46265 (comment)
There might be a better way to handle all kinds of host reaction jobs at once, but improving AsyncContext
support for thenables to match other patterns of Promise
and async
/await
usage seems like an improvement to me.
I know it's not this simple to change how V8 processes thenables/anything, with so many dependent projects (Node.js, Deno, Chromium, etc) who would all have to be on board (or indifferent). I also know I'm probably not showing the people who make these decisions anything they don't already know, but for me it's been useful to play with a prototype where AsyncContext
and thenables work together.
In other words, I don't claim to be fully addressing the last remaining concern expressed in cloudflare/workerd#282 (comment), but here's a REPL you can play with right now to see what I'm talking about:
docker run -it --rm benjamn/deno:async-thenables repl
Try pasting in this code:
const c = new AsyncContext();
c.run(1234, async () => {
console.log("before await", c.get());
const onResResult = await {
then(onRes) {
return Promise.resolve(onRes(c.get()));
}
};
console.log("after await", c.get(), { onResResult });
})
If all goes well, that code should log
before await 1234
after await 1234 { onResResult: 1234 }
Promise { undefined }
However, if you try the same code with my previous Docker image benjamn/deno:async-context
…
docker run -it --rm benjamn/deno:async-context repl
then the result will be
before await 1234
after await 1234 { onResResult: undefined }
Promise { undefined }
For more examples, see the test code I'm commenting on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, we were just talking about making these exact changes yesterday on our Matrix chat. If you're on Matrix, I'd love to add you to our group chat.
There might be a better way to handle all kinds of host reaction jobs at once, but improving AsyncContext support for thenables to match other patterns of Promise and async/await usage seems like an improvement to me.
Yes, I think instead of implementing these for each individual enqueable task type, the queue itself should implement this for all types. But for now, this fix is exactly what I recommended in our chat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're on Matrix, I'd love to add you to our group chat.
Sure! I'm a Matrix newb though I've used it a bit for TC39. I think @benjamn:matrix.org
is my handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're invited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this Matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr
These changes have been published in the
benjamn/deno:async-context
Docker image, building on the work in PR #1, meaning you should be able to run my custom build of Deno (if you have Docker installed) using the following command:docker run -it --rm benjamn/deno:async-context repl # Alternatively, for 'thenables' support: docker run -it --rm benjamn/deno:async-thenables repl
That command will open a JavaScript REPL where the
AsyncContext
constructor is available and fully implemented (according to my understanding of the proposal).Why now
I realize it's too early in the TC39 proposal stage process to be deciding anything about the implementation. I am not advocating for this specific implementation (nor promoting Deno or Docker) over any other good ideas.
However, this is a subtle proposal, and many of the interested parties bring intuitions from previous systems with goals similar to those motivating
AsyncContext
, but with slightly different concrete usage, semantics, pitfalls, etc.I hope this implementation allows those folks to check their intuitions thoroughly, especially in scenarios where the flow of
AsyncContext
could be needlessly lost (so explicitAsyncContext.wrap
binding may be necessary).Also, I believe everyone interested in these topics should at least be aware of
v8::Context::GetContinuationPreservedEmbedderData
andSetContinuationPreservedEmbedderData
, which provide a purpose-built way to hook into low-levelPromiseReactionJob
creation and execution, automatically bridging an all-important asynchronous gap in the ECMAScript event loop system.1Details
With this implementation,
AsyncContext
values are automatically preserved throughout the bodies of nativeasync
functions, even after one or moreawait
expressions, and also not exposed during asynchronous pauses to other unrelated code that might happen to be running on the event loop in the meantime.Similarly, typical use of
Promise
s (chainingthen
andcatch
andfinally
) should propagateAsyncContext
as expected. As a bonus,setTimeout
andsetInterval
now useAsyncContext.wrap
to ensure context continuity for their callbacks.As a concrete demonstration, these
expect
assertions pass:If you want something that's easier to paste into the Deno REPL, try this:
If all goes well, the console output should be:
Why did I choose Deno?
A few reasons, none of which conflict with my love of Node.js:
AsyncContext
proposal using the Node.jsasync_hooks
utilities (likeAsyncLocalStorage
), but I wanted to show that is not necessaryv8::Context
, which were introduced in this cryptic/convenient V8 pull request back in March 20202deno
binary)I'd be happy to try something similar with Node.js, if there's interest.
Why am I using Docker?
If you happen to have Docker installed, downloading and running a ready-to-go 162MB image is likely to be much faster than cloning the necessary repositories and submodules and building everything from source, which can take more than an hour on my late-model Intel Macbook Pro.
If you don't have access to Docker, the
docker/
directory is still a good reference for how to build the project from source.Building from source may also be worthwhile if you want to use the custom version of
deno
as yourdeno.path
in.vscode/settings.json
, so that (for example)AsyncContext
will be known by the type system to be globally defined when using the Deno VSCode integration.Footnotes
I don't think these
v8::Context::{Get,Set}ContinuationPreservedEmbedderData
methods should ever be exposed directly to userland code (global mutable state of the worst kind 😱), but I would argueAsyncContext
wraps a safe abstraction over that fundamental capability, achieving continuity ofAsyncContext
acrossPromiseReactionJob
creation and execution without exposing any of the necessary global state for tampering. ↩World events may have overshadowed the news of this pull request landing in V8 ↩