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

Patch v8 to support promise cross-context resolution #2719

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 17, 2024

Draft for now until I add tests tests and more tests

export default {
  async fetch(req, env, ctx) {
    if (globalThis.resolve === undefined) {

      {
        const { promise, resolve } = Promise.withResolvers();
        setTimeout(resolve, 1000);
        ctx.waitUntil(promise);
      }

      // This is our first request. We will create a new promise and resolver
      // pair and store it in a global. This request will then wait for the
      // promise to be resolved before continuing.
      const { promise, resolve } = Promise.withResolvers();
      globalThis.resolve = resolve;
      globalThis.promise = promise;
      const ab = AbortSignal.abort();
      console.log(ab.aborted);
      promise.then(() => {
        // This will be run within the correct IoContext now...
        try {
          console.log('test1', ab.aborted);
        } catch (err) {
          // We would get here if the IoContext was incorrect because
          // of the call to ab.aborted
          console.log(err.message);
        }
      });
    } else {
      // This is our second request. We will resolve the promise created in the
      // first request.
      console.log('....');
      globalThis.resolve();
      globalThis.resolve = undefined;
      console.log('test2');
    }

    return new Response("Hello World\n");
  }
};

Some details on how this works:

  • Per the original fixup, every IoContext has an associated opaque JS object called the "promise tag"
  • Originally, this promise tag was just a simple, ordinary JS object. With this PR it becomes an opaque wrapper around an kj::Own<IoContext::WeakRef>
  • When an IoContext becomes current/locked, that IoContexts promise tag is associated with the v8::Isolate as the "current promise tag"
  • Per the original fixup, whenever a Promise is created, the Isolate's "current promise tag" is associated with the Promise in an internal field.
  • In the original fixup, when a Promise is followed (.then(...), .catch(...), etc), we check to see if the current promise tag matches the promises context tag. If they are not the same, then the promise is a "cross-context promise" and we arrange for proper cross-context signaling.
  • This PR adds that when the resolve(...) or reject(...) methods are called, and we determine that the promise is a cross-context promise, scheduling of the promise continuations will be deferred until after we re-enter the promise's original IoContext.
  • If the promise's original IoContext has been destroyed, we drop the continuations on the floor and emit a warning.

@jasnell jasnell requested review from a team as code owners September 17, 2024 02:25
@jasnell jasnell marked this pull request as draft September 17, 2024 02:25
@jasnell
Copy link
Member Author

jasnell commented Sep 17, 2024

TODOs

  • Handle reject() ... Currently this only deals with the resolve case. Needs to handle the reject case also but that will follow the identical pattern.
  • Tests! Make sure all the various ways promises are resolved are handled. Async iterators, custom thenables, etc
  • Documentation ... We should add documentation to the website

@jasnell jasnell force-pushed the jsnell/resolve-context-patch branch 2 times, most recently from 5ff9282 to a8c8226 Compare September 17, 2024 15:34
@jasnell jasnell marked this pull request as ready for review September 17, 2024 15:34
@kentonv
Copy link
Member

kentonv commented Sep 17, 2024

The workerd C++ code seems fine to me. I'd like to defer to @erikcorry for the V8 side as I don't feel like I understand it.

Is there some way we can make it easier to review the patch? Maybe fork the v8 repo, commit it in your fork, and link the commit from here?

@jasnell

This comment was marked as resolved.

@jasnell
Copy link
Member Author

jasnell commented Sep 17, 2024

CI is failing due to a transient issue downloading stuff....

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved
@erikcorry
Copy link
Contributor

Here's a diff to the diff file, which uses Handles more, and raw Tagged pointers less. Feel free to use it, or I'll just do a separate change tomorrow (where I'll probably also unify the 0007 and 0019 patches to one file).
handles.txt

Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

Not quite done reviewing but need to go to meetings, so here's my comments so far.

src/workerd/io/io-own.h Outdated Show resolved Hide resolved
src/workerd/io/io-own.c++ Outdated Show resolved Hide resolved
src/workerd/io/io-own.c++ Outdated Show resolved Hide resolved
src/workerd/io/io-own.h Outdated Show resolved Hide resolved
src/workerd/io/io-own.c++ Outdated Show resolved Hide resolved
src/workerd/io/io-own.h Outdated Show resolved Hide resolved
Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

OK I actually read through everything this time (except the V8 patch).

src/workerd/io/io-own.h Outdated Show resolved Hide resolved
src/workerd/io/worker.c++ Outdated Show resolved Hide resolved
src/workerd/io/io-context.c++ Outdated Show resolved Hide resolved
src/workerd/io/io-context.c++ Outdated Show resolved Hide resolved
src/workerd/io/io-context.c++ Outdated Show resolved Hide resolved
src/workerd/io/worker.c++ Outdated Show resolved Hide resolved
src/workerd/io/worker.c++ Show resolved Hide resolved
Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

Approving workerd code... trusting @erikcorry to review V8 code.

@jasnell
Copy link
Member Author

jasnell commented Sep 18, 2024

Rebased and squashed

Copy link
Contributor

@erikcorry erikcorry left a comment

Choose a reason for hiding this comment

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

LGTM

```
export default {
  async fetch(req, env, ctx) {
    if (globalThis.resolve === undefined) {

      {
        const { promise, resolve } = Promise.withResolvers();
        setTimeout(resolve, 1000);
        ctx.waitUntil(promise);
      }

      // This is our first request. We will create a new promise and resolver
      // pair and store it in a global. This request will then wait for the
      // promise to be resolved before continuing.
      const { promise, resolve } = Promise.withResolvers();
      globalThis.resolve = resolve;
      globalThis.promise = promise;
      const ab = AbortSignal.abort();
      console.log(ab.aborted);
      promise.then(() => {
        // This will be run within the correct IoContext now...
        try {
          console.log('test1', ab.aborted);
        } catch (err) {
          // We would get here if the IoContext was incorrect because
          // of the call to ab.aborted
          console.log(err.message);
        }
      });
    } else {
      // This is our second request. We will resolve the promise created in the
      // first request.
      console.log('....');
      globalThis.resolve();
      globalThis.resolve = undefined;
      console.log('test2');
    }

    return new Response("Hello World\n");
  }
};

```
@jasnell jasnell merged commit 6f43bbc into main Sep 18, 2024
12 of 14 checks passed
@jasnell jasnell deleted the jsnell/resolve-context-patch branch September 18, 2024 20:50
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.

3 participants