Skip to content

Commit

Permalink
Patch v8 to support promise cross-context resolution
Browse files Browse the repository at this point in the history
```
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");
  }
};

```
  • Loading branch information
jasnell committed Sep 17, 2024
1 parent 488b490 commit 5ff9282
Show file tree
Hide file tree
Showing 9 changed files with 750 additions and 1 deletion.
1 change: 1 addition & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,7 @@ http_archive(
"//:patches/v8/0016-Revert-TracedReference-deref-API-removal.patch",
"//:patches/v8/0017-Revert-heap-Add-masm-specific-unwinding-annotations-.patch",
"//:patches/v8/0018-Update-illegal-invocation-error-message-in-v8.patch",
"//:patches/v8/0019-Implement-cross-request-context-promise-resolve-hand.patch",
],
strip_prefix = "v8-12.9.202.13",
url = "https://github.com/v8/v8/archive/refs/tags/12.9.202.13.tar.gz",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,339 @@
From 30aad178c5b0fda89e57483d02bd2e3cb4304c22 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Mon, 16 Sep 2024 09:56:04 -0700
Subject: [PATCH] Implement cross-request context promise resolve handling

---
include/v8-callbacks.h | 21 +++++++++++
include/v8-isolate.h | 2 ++
src/api/api.cc | 5 +++
src/builtins/promise-abstract-operations.tq | 6 +++-
src/builtins/promise-resolve.tq | 4 ++-
src/execution/isolate-inl.h | 9 +++++
src/execution/isolate.cc | 22 ++++++++++++
src/execution/isolate.h | 14 +++++++-
src/objects/js-promise.h | 5 +++
src/objects/objects.cc | 39 +++++++++++++++++++++
src/runtime/runtime-promise.cc | 18 ++++++++++
src/runtime/runtime.h | 3 +-
12 files changed, 144 insertions(+), 4 deletions(-)

diff --git a/include/v8-callbacks.h b/include/v8-callbacks.h
index 25f0c8189e5..df7324d4457 100644
--- a/include/v8-callbacks.h
+++ b/include/v8-callbacks.h
@@ -471,6 +471,27 @@ using PromiseCrossContextCallback = MaybeLocal<Promise> (*)(Local<Context> conte
Local<Promise> promise,
Local<Object> tag);

+/**
+ * PromiseCrossContextResolveCallback is called when resolving or rejecting a
+ * pending promise whose context tag is not strictly equal to the isolate's
+ * current promise context tag. The callback is called with the promise to be
+ * resolved, it's context tag, and a function that when called, causes the
+ * reactions to the resolved promise to be enqueued. The idea is that the
+ * embedder sets this callback in the case it needs to defer the actual
+ * scheduling of the reactions to the given promise to a later time.
+ * Importantly, when this callback is invoked, the state of the promise
+ * should have already been updated. We're simply possibly deferring the
+ * enqueue of the reactions to the promise.
+ */
+using PromiseCrossContextResolveCallback = void (*)(
+ v8::Isolate* isolate,
+ Local<Value> tag,
+ Local<Value> reactions,
+ Local<Value> argument,
+ std::function<void(v8::Isolate* isolate,
+ Local<Value> reactions,
+ Local<Value> argument)> callback);
+
} // namespace v8

#endif // INCLUDE_V8_ISOLATE_CALLBACKS_H_
diff --git a/include/v8-isolate.h b/include/v8-isolate.h
index 8112231319b..9fe57f7bde4 100644
--- a/include/v8-isolate.h
+++ b/include/v8-isolate.h
@@ -1732,6 +1732,8 @@ class V8_EXPORT Isolate {

class PromiseContextScope;
void SetPromiseCrossContextCallback(PromiseCrossContextCallback callback);
+ void SetPromiseCrossContextResolveCallback(
+ PromiseCrossContextResolveCallback callback);

Isolate() = delete;
~Isolate() = delete;
diff --git a/src/api/api.cc b/src/api/api.cc
index d3fc6d77ea3..3255fd77e25 100644
--- a/src/api/api.cc
+++ b/src/api/api.cc
@@ -12246,6 +12246,11 @@ void Isolate::SetPromiseCrossContextCallback(PromiseCrossContextCallback callbac
isolate->set_promise_cross_context_callback(callback);
}

+void Isolate::SetPromiseCrossContextResolveCallback(PromiseCrossContextResolveCallback callback) {
+ i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
+ isolate->set_promise_cross_context_resolve_callback(callback);
+}
+
Isolate::PromiseContextScope::PromiseContextScope(Isolate* isolate, v8::Local<v8::Object> tag)
: isolate_(reinterpret_cast<i::Isolate*>(isolate)) {
DCHECK(!isolate_->has_promise_context_tag());
diff --git a/src/builtins/promise-abstract-operations.tq b/src/builtins/promise-abstract-operations.tq
index fdec3d268e4..456d051d3c0 100644
--- a/src/builtins/promise-abstract-operations.tq
+++ b/src/builtins/promise-abstract-operations.tq
@@ -23,6 +23,9 @@ extern transitioning runtime PromiseRejectEventFromStack(

extern transitioning runtime PromiseContextCheck(
implicit context: Context)(JSPromise): JSPromise;
+
+extern transitioning runtime PromiseResolveContextCheck(
+ implicit context: Context)(JSPromise): JSAny;
}

// https://tc39.es/ecma262/#sec-promise-abstract-operations
@@ -239,7 +242,8 @@ transitioning builtin RejectPromise(
// the runtime handle this operation, which greatly reduces
// the complexity here and also avoids a couple of back and
// forth between JavaScript and C++ land.
- if (IsIsolatePromiseHookEnabledOrDebugIsActiveOrHasAsyncEventDelegate(
+ if (ToBoolean(runtime::PromiseResolveContextCheck(promise)) ||
+ IsIsolatePromiseHookEnabledOrDebugIsActiveOrHasAsyncEventDelegate(
promiseHookFlags) ||
!promise.HasHandler()) {
// 7. If promise.[[PromiseIsHandled]] is false, perform
diff --git a/src/builtins/promise-resolve.tq b/src/builtins/promise-resolve.tq
index 202180adbba..c93ac5905d7 100644
--- a/src/builtins/promise-resolve.tq
+++ b/src/builtins/promise-resolve.tq
@@ -96,7 +96,9 @@ transitioning builtin ResolvePromise(
// We also let the runtime handle it if promise == resolution.
// We can use pointer comparison here, since the {promise} is guaranteed
// to be a JSPromise inside this function and thus is reference comparable.
- if (IsIsolatePromiseHookEnabledOrDebugIsActiveOrHasAsyncEventDelegate() ||
+
+ if (ToBoolean(runtime::PromiseResolveContextCheck(promise)) ||
+ IsIsolatePromiseHookEnabledOrDebugIsActiveOrHasAsyncEventDelegate() ||
TaggedEqual(promise, resolution))
deferred {
return runtime::ResolvePromise(promise, resolution);
diff --git a/src/execution/isolate-inl.h b/src/execution/isolate-inl.h
index 9e0969460fd..1084adfa7f5 100644
--- a/src/execution/isolate-inl.h
+++ b/src/execution/isolate-inl.h
@@ -154,6 +154,15 @@ void Isolate::set_promise_cross_context_callback(PromiseCrossContextCallback cal
promise_cross_context_callback_ = callback;
}

+void Isolate::set_promise_cross_context_resolve_callback(
+ PromiseCrossContextResolveCallback callback) {
+ promise_cross_context_resolve_callback_ = callback;
+}
+
+bool Isolate::has_promise_context_resolve_callback() {
+ return promise_cross_context_resolve_callback_ != nullptr;
+}
+
#ifdef DEBUG
Tagged<Object> Isolate::VerifyBuiltinsResult(Tagged<Object> result) {
if (is_execution_terminating() && !v8_flags.strict_termination_checks) {
diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc
index 94194d59bae..920a0efd797 100644
--- a/src/execution/isolate.cc
+++ b/src/execution/isolate.cc
@@ -7459,5 +7459,27 @@ MaybeHandle<JSPromise> Isolate::RunPromiseCrossContextCallback(Handle<NativeCont
return v8::Utils::OpenHandle(*result);
}

+void Isolate::RunPromiseCrossContextResolveCallback(v8::Isolate* isolate,
+ Handle<JSObject> tag,
+ Handle<Object> reactions,
+ Handle<Object> argument,
+ PromiseReaction::Type type) {
+ CHECK(promise_cross_context_resolve_callback_ != nullptr);
+ promise_cross_context_resolve_callback_(
+ isolate,
+ v8::Utils::ToLocal(tag),
+ v8::Utils::ToLocal(reactions),
+ v8::Utils::ToLocal(argument),
+ [type](v8::Isolate* isolate,
+ v8::Local<v8::Value> reactions,
+ v8::Local<v8::Value> argument) {
+ JSPromise::ContinueTriggerPromiseReactions(
+ reinterpret_cast<Isolate*>(isolate),
+ Utils::OpenHandle(*reactions),
+ Utils::OpenHandle(*argument),
+ type);
+ });
+}
+
} // namespace internal
} // namespace v8
diff --git a/src/execution/isolate.h b/src/execution/isolate.h
index 59926e68e97..2bd24b914b0 100644
--- a/src/execution/isolate.h
+++ b/src/execution/isolate.h
@@ -42,6 +42,7 @@
#include "src/objects/contexts.h"
#include "src/objects/debug-objects.h"
#include "src/objects/js-objects.h"
+#include "src/objects/promise.h"
#include "src/objects/tagged.h"
#include "src/runtime/runtime.h"
#include "src/sandbox/code-pointer-table.h"
@@ -2249,8 +2250,17 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
inline void clear_promise_context_tag();
inline void set_promise_context_tag(Tagged<Object> tag);
inline void set_promise_cross_context_callback(PromiseCrossContextCallback callback);
+ inline void set_promise_cross_context_resolve_callback(
+ PromiseCrossContextResolveCallback callback);
MaybeHandle<JSPromise> RunPromiseCrossContextCallback(Handle<NativeContext> context,
Handle<JSPromise> promise);
+ void RunPromiseCrossContextResolveCallback(v8::Isolate* isolate,
+ Handle<JSObject> tag,
+ Handle<Object> reactions,
+ Handle<Object> argument,
+ PromiseReaction::Type type);
+
+ inline bool has_promise_context_resolve_callback();

#ifdef V8_ENABLE_WASM_SIMD256_REVEC
void set_wasm_revec_verifier_for_test(
@@ -2764,8 +2774,10 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
#endif

Tagged<Object> promise_context_tag_;
- PromiseCrossContextCallback promise_cross_context_callback_;
+ PromiseCrossContextCallback promise_cross_context_callback_ = nullptr;
+ PromiseCrossContextResolveCallback promise_cross_context_resolve_callback_ = nullptr;
bool in_promise_cross_context_callback_ = false;
+ bool in_promise_cross_context_resolve_callback_ = false;

class PromiseCrossContextCallbackScope;

diff --git a/src/objects/js-promise.h b/src/objects/js-promise.h
index e4c1e42c63e..87bebb70d0d 100644
--- a/src/objects/js-promise.h
+++ b/src/objects/js-promise.h
@@ -75,6 +75,11 @@ class JSPromise
static_assert(v8::Promise::kFulfilled == 1);
static_assert(v8::Promise::kRejected == 2);

+ static void ContinueTriggerPromiseReactions(Isolate* isolate,
+ DirectHandle<Object> reactions,
+ DirectHandle<Object> argument,
+ PromiseReaction::Type type);
+
private:
// ES section #sec-triggerpromisereactions
static Handle<Object> TriggerPromiseReactions(Isolate* isolate,
diff --git a/src/objects/objects.cc b/src/objects/objects.cc
index 76e284e19a2..32ccc84d8f8 100644
--- a/src/objects/objects.cc
+++ b/src/objects/objects.cc
@@ -4878,6 +4878,21 @@ Handle<Object> JSPromise::Fulfill(DirectHandle<JSPromise> promise,
// 6. Set promise.[[PromiseState]] to "fulfilled".
promise->set_status(Promise::kFulfilled);

+ Tagged<Object> obj = promise->context_tag();
+ bool needs_promise_context_switch =
+ !(obj == Smi::zero() ||
+ obj == isolate->promise_context_tag() ||
+ !isolate->has_promise_context_resolve_callback());
+ if (needs_promise_context_switch) {
+ isolate->RunPromiseCrossContextResolveCallback(
+ reinterpret_cast<v8::Isolate*>(isolate),
+ Handle<JSObject>(Cast<JSObject>(obj), isolate),
+ reactions,
+ value,
+ PromiseReaction::kFulfill);
+ return isolate->factory()->undefined_value();
+ }
+
// 7. Return TriggerPromiseReactions(reactions, value).
return TriggerPromiseReactions(isolate, reactions, value,
PromiseReaction::kFulfill);
@@ -4933,6 +4948,21 @@ Handle<Object> JSPromise::Reject(Handle<JSPromise> promise,
isolate->ReportPromiseReject(promise, reason, kPromiseRejectWithNoHandler);
}

+ Tagged<Object> obj = promise->context_tag();
+ bool needs_promise_context_switch =
+ !(obj == Smi::zero() ||
+ obj == isolate->promise_context_tag() ||
+ !isolate->has_promise_context_resolve_callback());
+ if (needs_promise_context_switch) {
+ isolate->RunPromiseCrossContextResolveCallback(
+ reinterpret_cast<v8::Isolate*>(isolate),
+ Handle<JSObject>(Cast<JSObject>(obj), isolate),
+ reactions,
+ reason,
+ PromiseReaction::kReject);
+ return isolate->factory()->undefined_value();
+ }
+
// 8. Return TriggerPromiseReactions(reactions, reason).
return TriggerPromiseReactions(isolate, reactions, reason,
PromiseReaction::kReject);
@@ -5035,6 +5065,15 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
}

// static
+
+void JSPromise::ContinueTriggerPromiseReactions(
+ Isolate* isolate,
+ DirectHandle<Object> reactions,
+ DirectHandle<Object> argument,
+ PromiseReaction::Type type) {
+ TriggerPromiseReactions(isolate, reactions, argument, type);
+}
+
Handle<Object> JSPromise::TriggerPromiseReactions(
Isolate* isolate, DirectHandle<Object> reactions,
DirectHandle<Object> argument, PromiseReaction::Type type) {
diff --git a/src/runtime/runtime-promise.cc b/src/runtime/runtime-promise.cc
index 4fbdf90319c..bb4e9c7fe41 100644
--- a/src/runtime/runtime-promise.cc
+++ b/src/runtime/runtime-promise.cc
@@ -251,5 +251,23 @@ RUNTIME_FUNCTION(Runtime_PromiseContextCheck) {
return *result;
}

+RUNTIME_FUNCTION(Runtime_PromiseResolveContextCheck) {
+ HandleScope scope(isolate);
+ DCHECK_EQ(1, args.length());
+ Handle<JSPromise> promise = args.at<JSPromise>(0);
+ // If promise.context_tag() is strict equal to isolate.promise_context_tag(),
+ // or if the promise being checked does not have a context tag, or if the
+ // resolve callback has not been set, we'll just return false here to indicate
+ // that the default handling should be used.
+ Tagged<Object> obj = promise->context_tag();
+ if (obj == Smi::zero() ||
+ obj == isolate->promise_context_tag() ||
+ !isolate->has_promise_context_resolve_callback()) {
+ return isolate->heap()->ToBoolean(false);
+ }
+
+ return isolate->heap()->ToBoolean(true);
+}
+
} // namespace internal
} // namespace v8
diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h
index 9399509f690..bfca9182be0 100644
--- a/src/runtime/runtime.h
+++ b/src/runtime/runtime.h
@@ -403,7 +403,8 @@ namespace internal {
F(ConstructAggregateErrorHelper, 4, 1) \
F(ConstructInternalAggregateErrorHelper, -1 /* <= 5*/, 1) \
F(PromiseContextInit, 1, 1) \
- F(PromiseContextCheck, 1, 1)
+ F(PromiseContextCheck, 1, 1) \
+ F(PromiseResolveContextCheck, 1, 1)

#define FOR_EACH_INTRINSIC_PROXY(F, I) \
F(CheckProxyGetSetTrapResult, 2, 1) \
--
2.34.1

6 changes: 6 additions & 0 deletions src/workerd/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -540,3 +540,9 @@ wd_test(
args = ["--experimental"],
data = ["tests/new-module-registry-test.js"],
)

wd_test(
src = "tests/cross-context-promise-test.wd-test",
args = ["--experimental"],
data = ["tests/cross-context-promise-test.js"],
)
Loading

0 comments on commit 5ff9282

Please sign in to comment.