Skip to content
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

Proof of concept: native prototype of AsyncContext proposal using existing v8::Context methods #17

Closed
wants to merge 1 commit into from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jan 23, 2023

Hello @legendecas, @jridgewell, and other interested parties!

First of all, this proposal has my enthusiastic support, even/especially in its current, relatively minimal form. I’d love to talk about the kinds of context systems that can be built on this foundation, but that’s a topic for another thread.

In case this is helpful for your demonstration purposes, I wanted to let you know about a fully functional1 native implementation of AsyncContext that I’ve published as a cross-platform Docker image: benjamn/deno:async-context. Of course, if you already have your own native implementation strategy, I’d love to see how it works and compare notes.

If you have Docker installed, you can fire up a REPL where AsyncContext is defined using the following command:

docker run -it --rm benjamn/deno:async-context repl

# Alternatively, for improved 'thenables' support:
docker run -it --rm benjamn/deno:async-thenables repl

You can also build the project from source, but that takes much longer (>60min on a late model Intel Macbook pro), mostly because V8 has to be built from source, even though I am not modifying the V8 source in any way. The scripts used to build the Docker image are probably the best reference for how to build it locally, if you prefer that route.

Thanks to the way this implementation hooks into the creation and execution of PromiseReactionJob objects, native async/await and Promise code like this automatically works, with no modifications to Promise.prototype.then:

const ctx = new AsyncContext;
ctx.run(1234, async () => {
  console.log("before await", ctx.get());
  const setTimeoutResult = await ctx.run(
    2345,
    () => new Promise(resolve => {
      setTimeout(() => resolve(ctx.get()), 20);
    }),
  );
  console.log("after await", ctx.get());
  console.log({ setTimeoutResult });
  return "final result";
}).then(result => {
  console.log("final result?", result);
  console.log("outside ctx.run", ctx.get());
})

This prints

before await 1234
Promise { <pending> }
> after await 1234
{ setTimeoutResult: 2345 }
final result? final result
outside ctx.run undefined

I adapted the existing tests from this repository here. After some predictable adjustments to the imports and removing the Promise.prototype.then wrapping code, those tests are all passing. This PR also contains a version of this Docker-powered native test suite, in case you want to try adding your own tests (feel free to push to my branch).2

As a bonus, setTimeout and setInterval in this implementation use AsyncContext.wrap for their callbacks to achieve AsyncContext preservation. This is a good example of how the host environment can work with AsyncContext when implementing its own runtime APIs (like setTimeout) that are not part of the ECMAScript specification.

I realize it’s way too early in the TC39 proposal process to commit to any specific implementation details, but this is a subtle proposal, and many of the interested parties bring intuitions from previous systems with similar goals but slightly different concrete usage, semantics, pitfalls, etc. I hope this native implementation allows those folks to check their intuitions thoroughly, especially in scenarios where the desired flow of AsyncContext could be broken. When you no longer have to keep in mind the limitations of the Promise.prototype.then polyfill from #14, it's easier to appreciate the unification of synchronous and asynchronous context management that this proposal allows.

As I explained in benjamn/deno#2, I’m using Deno because it’s a V8-based runtime that does not already have the context-like functionality Node.js has been adding with async_hooks. I imagine the Node.js implementation of AsyncContext will attempt to use or interoperate with async_hooks, but I wanted to show you don’t need that foundation; hence Deno. All you need is a way to hook into PromiseReactionJob creation/execution, which V8 already provides via two existing methods of v8::Context, introduced in this cryptic/convenient V8 pull request back in March 2020.

Please let me know if you run into any snags with this implementation, or you have any other questions or suggestions for improvement. Looking forward to your presentation next week!

Footnotes

  1. I believe this implementation is complete and correct at the moment, but I would encourage you to play with it yourselves before recommending it to others, just in case I’m missing something.

  2. One of the maintainers may need to approve/enable the .github/workflows/docker-test.sh workflow to allow it to run for this PR.

Comment on lines +4 to +27
describe("async via native async/await", () => {
it("works after awaited setTimeout result", async () => {
const ctx = new AsyncContext<number>();
const ctxRunResult = await ctx.run(1234, async () => {
assert.strictEqual(ctx.get(), 1234);
const setTimeoutResult = await ctx.run(
2345,
() => new Promise(resolve => {
setTimeout(() => resolve(ctx.get()), 20);
}),
);
assert.strictEqual(setTimeoutResult, 2345);
assert.strictEqual(ctx.get(), 1234);
return "final result";
}).then(result => {
assert.strictEqual(result, "final result");
// The code that generated the Promise has access to the 1234 value
// provided to ctx.run above, but consumers of the Promise do not
// automatically inherit it.
assert.strictEqual(ctx.get(), void 0);
return "ctx.run result 👋";
});
assert.strictEqual(ctxRunResult, "ctx.run result 👋");
});
Copy link
Member Author

@benjamn benjamn Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please feel free to add more native tests here (pushing to my branch is welcome).

If the maintainers approve/enable my .github/workflows/docker-test.sh GitHub Action, the tests should automatically run with each commit pushed to this PR.

@benjamn
Copy link
Member Author

benjamn commented Jan 23, 2023

@lucacasonato Since you represent Deno on TC39, I thought you might appreciate a heads up about this prototype, which uses Deno. Any feedback welcome!

@benjamn benjamn force-pushed the benjamn/native-prototype branch from 536cfb8 to fb034c9 Compare January 23, 2023 17:35
@jridgewell
Copy link
Member

Thanks for the PR! I haven't reviewed yet (I will today!), but I also want to point you to https://github.com/jridgewell/async-context-native and https://github.com/jridgewell/engine262/tree/async-context implementations. We should probably collect all the implementations

@benjamn
Copy link
Member Author

benjamn commented Jan 23, 2023

Ahh, thank you for linking me to nodejs/node#46265. It's great to see these ideas rippling through the Node.js project already.

@benjamn
Copy link
Member Author

benjamn commented Jan 25, 2023

@jridgewell Did you ever hunt down a solution to the thenables problem? I think I've got one, thanks to you pointing out those bugs with V8's continuation support: benjamn/deno#2 (review)

docker run -it --rm benjamn/deno:async-thenables repl

@jridgewell
Copy link
Member

@benjamn Are you interested in submitting your thenable patch to V8?

@Jbnado
Copy link

Jbnado commented Mar 1, 2023

I would like to understand more, so this comment is more a question than something that could help, so @benjamn in this PR you're updating the tests removing thenables and showing a docker image that you implemented a context api but @jridgewell said that you're changes could not be right because a bug in V8 engine and then you fixed it?

@benjamn
Copy link
Member Author

benjamn commented Mar 6, 2023

Thanks for asking @Jbnado! Let me try to explain the situation as I see it…

A faithful implementation of AsyncContext (one that works inside native async functions and with normal Promise usage) requires native support. You can get close with userland/library implementations like #14, but your running program will typically lose context after any await expression, and you might have to wrap/bind functions somehow before passing them to Promise.prototype.then and .catch. For example, you might need promise.then(AsyncContext.wrap(result => { ... })) instead of just promise.then(result => { ... }).

The reason context is lost in these scenarios is they involve scheduling asynchronous events/tasks/jobs on an event loop, and context1 is not automatically inherited from one tick of the event loop to the next, at least not in JavaScript today.

As it turns out, the V8 JavaScript runtime has two internal v8::Context methods (not available at runtime, but available to native extensions and programs that embed V8, like Node.js and Deno), GetContinuationPreservedEmbedderData and SetContinuationPreservedEmbedderData, that can be used to implement a more faithful version of AsyncContext, avoiding the pitfalls described above.

Though it is not obvious from their naming, these methods participate in a system whose goal is to capture a specific value (the "continuation-preserved embedder data") whenever an event loop job is created (which typically corresponds to when the job is scheduled on the event loop), and restore (or "preserve") that value when the event is eventually invoked. The eventual invocation of the scheduled job callback "continues" the work scheduled by the previous job, hence the continuation terminology.2 Calling SetContinuationPreservedEmbedderData allows setting the embedder data, which gets picked up when event loop jobs are created/scheduled. When those jobs later run, the embedder data from the time of their scheduling will be restored, so child jobs may use GetContinuationPreservedEmbedderData to obtain the embedder data and continue the work of the parent job.3

While this system is agnostic about what the embedder data represents and how it should be used, it's extremely tempting to use it for some kind of inheritable event loop context mechanism, which is why I thought it was a good choice for a prototype implementation of the AsyncContext proposal.

I believe @jridgewell and I agree that the problem {Get,Set}ContinuationPreservedEmbedderData were designed to solve is essentially the same problem AsyncContext needs to solve: ensuring every host reaction job inherits context from its scheduling job.

However, Justin pointed out some valid criticisms in #17 (comment), suggesting the current embedder data system in V8 has some bugs that prevent it from solving this problem perfectly, because the embedder data can fail to be preserved across continuations in some cases, such as when you're using a "thenable" object (not a Promise but an object with a promise-like then method).

and then you fixed it?

Yes, I was able to fix that particular V8 shortcoming in my fork, as described in benjamn/deno#2 (comment).

In response to #17 (comment), I am still working on that PR, but writing the necessary C++ tests (that fail without my change and pass with it applied) has proven challenging 😅

While I have your attention (thanks for reading this far!), I also have to point out another big problem with v8::Context::{Get,Set}ContinuationPreservedEmbedderData: if anyone else is already using the embedder data, then no one else can safely use it. More than any other reason, this is why we cannot safely ship support for AsyncContext in V8 using embedder data. Ultimately, we need to find a way to make embedder data serve many independent consumers.

The good news is that AsyncContext already has this important property: anyone can create their own private AsyncContext instance, and its value can be manipulated independently of all the others (though they all get preserved together across event loop jobs). In other words, AsyncContext has a chance to be the API that v8::Context::{Get,Set}ContinuationPreservedEmbedderData should have been, and I believe AsyncContext will eventually replace all usage of those internal V8 APIs in the wild, once/if this proposal becomes part of the ECMAScript spec.

Footnotes

  1. The word "context" has multiple different meanings even within the JavaScript specification, and countless other (context-dependent!) meanings in the wider world. In order to talk about preserving context, we must first decide how to represent it, which is no small undertaking. AsyncContext is one such attempt to turn context into something that can be programmatically manipulated, an important first step toward preventing event loop context loss.

  2. Continuations are a more general idea than callback functions, as any Scheme programmer can tell you, but callbacks are a common representation of continuations in JavaScript.

  3. JavaScript code cannot call v8::Context::{Get,Set}ContinuationPreservedEmbedderData directly, but native code can (e.g. code inside Node.js or Deno), and JS APIs (like AsyncContext) can be built around such native code, so application code may end up "calling" these methods indirectly.

@littledan
Copy link
Member

This is great work, @benjamn ! I want to suggest that you land it in another repository (which contains disclaimers about this proposal's experimental nature, as well as the instability and noncomposability of this V8 API), and then we can link it from the README of this proposal.

@benjamn
Copy link
Member Author

benjamn commented Mar 14, 2023

Great idea @littledan. I agree this PR doesn't belong here in exactly this current form, so I will reopen a separate/simpler PR with an external link.

@benjamn benjamn closed this Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants