From 37e4a9bb616834801cf77952f9707e943ac23801 Mon Sep 17 00:00:00 2001 From: Joe Lee Date: Tue, 4 Jun 2024 09:39:32 -0700 Subject: [PATCH] Revert "Revert "Add .retryable and .overloaded properties to tunneled exception objects"" This reverts commit 051dbfcf2b06b997c566ea94b584e1ff122c1798. --- src/workerd/api/streams/standard-test.c++ | 2 ++ src/workerd/jsg/BUILD.bazel | 1 + src/workerd/jsg/jsg-test.h | 5 ++- src/workerd/jsg/modules-new-test.c++ | 2 ++ src/workerd/jsg/util.c++ | 40 ++++++++++++++++++++--- src/workerd/jsg/value.h | 22 +++++++++++-- src/workerd/util/autogate.c++ | 11 +++++++ src/workerd/util/autogate.h | 5 +++ 8 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/workerd/api/streams/standard-test.c++ b/src/workerd/api/streams/standard-test.c++ index 702109e86fc..5bb1b42d682 100644 --- a/src/workerd/api/streams/standard-test.c++ +++ b/src/workerd/api/streams/standard-test.c++ @@ -3,6 +3,7 @@ #include #include #include +#include namespace workerd::api { namespace { @@ -15,6 +16,7 @@ struct RsContext: public jsg::Object, public jsg::ContextGlobal { JSG_DECLARE_ISOLATE_TYPE(RsIsolate, RsContext, ReadResult); void preamble(auto callback) { + util::Autogate::initEmptyAutogateForTesting(); RsIsolate isolate(v8System, kj::heap()); isolate.runInLockScope([&](RsIsolate::Lock& lock) { JSG_WITHIN_CONTEXT_SCOPE(lock, diff --git a/src/workerd/jsg/BUILD.bazel b/src/workerd/jsg/BUILD.bazel index 3b48054a3b7..2e20fbac28b 100644 --- a/src/workerd/jsg/BUILD.bazel +++ b/src/workerd/jsg/BUILD.bazel @@ -58,6 +58,7 @@ wd_cc_library( ":observer", ":url", "//src/workerd/util", + "//src/workerd/util:autogate", "//src/workerd/util:sentry", "//src/workerd/util:thread-scopes", "@capnp-cpp//src/kj", diff --git a/src/workerd/jsg/jsg-test.h b/src/workerd/jsg/jsg-test.h index cc85aab98dc..4d92d9df237 100644 --- a/src/workerd/jsg/jsg-test.h +++ b/src/workerd/jsg/jsg-test.h @@ -8,6 +8,7 @@ #include "jsg.h" #include "setup.h" #include +#include namespace workerd::jsg::test { @@ -26,7 +27,9 @@ class Evaluator { // in cases that the isolate includes types that require configuration, but currently the // type is always default-constructed. What if you want to specify a test config? public: - explicit Evaluator(V8System& v8System) : v8System(v8System) {} + explicit Evaluator(V8System& v8System) : v8System(v8System) { + util::Autogate::initEmptyAutogateForTesting(); + } IsolateType& getIsolate() { // Slightly more efficient to only instantiate each isolate type once (17s vs. 20s): diff --git a/src/workerd/jsg/modules-new-test.c++ b/src/workerd/jsg/modules-new-test.c++ index d2e8f27b0d9..a41b9cce42b 100644 --- a/src/workerd/jsg/modules-new-test.c++ +++ b/src/workerd/jsg/modules-new-test.c++ @@ -5,6 +5,7 @@ #include #include #include +#include #include "observer.h" #include "url.h" #include @@ -108,6 +109,7 @@ struct TestContext: public Object, public ContextGlobal { JSG_DECLARE_ISOLATE_TYPE(TestIsolate, TestContext, TestType); #define PREAMBLE(fn) \ + util::Autogate::initEmptyAutogateForTesting(); \ TestIsolate isolate(v8System, 123, kj::heap()); \ runInV8Stack([&](auto& stackScope) { \ TestIsolate::Lock lock(isolate, stackScope); \ diff --git a/src/workerd/jsg/util.c++ b/src/workerd/jsg/util.c++ index 31812ea8f80..726545fbb3f 100644 --- a/src/workerd/jsg/util.c++ +++ b/src/workerd/jsg/util.c++ @@ -111,6 +111,26 @@ bool setRemoteError(v8::Isolate* isolate, v8::Local& exception) { v8::True(isolate))); } +bool setRetryableError(v8::Isolate* isolate, v8::Local& exception) { + KJ_ASSERT(exception->IsObject()); + auto obj = exception.As(); + return jsg::check( + obj->Set( + isolate->GetCurrentContext(), + jsg::v8StrIntern(isolate, "retryable"_kj), + v8::True(isolate))); +} + +bool setOverloadedError(v8::Isolate* isolate, v8::Local& exception) { + KJ_ASSERT(exception->IsObject()); + auto obj = exception.As(); + return jsg::check( + obj->Set( + isolate->GetCurrentContext(), + jsg::v8StrIntern(isolate, "overloaded"_kj), + v8::True(isolate))); +} + bool setDurableObjectResetError(v8::Isolate* isolate, v8::Local& exception) { KJ_ASSERT(exception->IsObject()); auto obj = exception.As(); @@ -128,7 +148,8 @@ struct DecodedException { }; DecodedException decodeTunneledException(v8::Isolate* isolate, - kj::StringPtr internalMessage) { + kj::StringPtr internalMessage, + kj::Exception::Type excType) { // We currently support tunneling the following error types: // // - Error: While the Web IDL spec claims this is reserved for use by program authors, this @@ -204,6 +225,14 @@ DecodedException decodeTunneledException(v8::Isolate* isolate, setRemoteError(isolate, result.handle); } + if (util::Autogate::isEnabled(util::AutogateKey::ACTOR_EXCEPTION_PROPERTIES)) { + if (excType == kj::Exception::Type::DISCONNECTED) { + setRetryableError(isolate, result.handle); + } else if (excType == kj::Exception::Type::OVERLOADED) { + setOverloadedError(isolate, result.handle); + } + } + if (result.isDurableObjectReset) { setDurableObjectResetError(isolate, result.handle); } @@ -222,8 +251,6 @@ kj::StringPtr extractTunneledExceptionDescription(kj::StringPtr message) { } } - - v8::Local makeInternalError(v8::Isolate* isolate, kj::Exception&& exception) { auto desc = exception.getDescription(); @@ -235,7 +262,7 @@ v8::Local makeInternalError(v8::Isolate* isolate, kj::Exception&& exc // in order to extract a full stack trace. Once we do it here, we can remove the code from // there. - auto tunneledException = decodeTunneledException(isolate, desc); + auto tunneledException = decodeTunneledException(isolate, desc, exception.getType()); if (tunneledException.isInternal) { auto& observer = IsolateBase::from(isolate).getObserver(); @@ -259,6 +286,11 @@ v8::Local makeInternalError(v8::Isolate* isolate, kj::Exception&& exc setRemoteError(isolate, exception); } + if (util::Autogate::isEnabled(util::AutogateKey::ACTOR_EXCEPTION_PROPERTIES)) { + // DISCONNECTED exceptions are considered retryable + setRetryableError(isolate, exception); + } + if (tunneledException.isDurableObjectReset) { setDurableObjectResetError(isolate, exception); } diff --git a/src/workerd/jsg/value.h b/src/workerd/jsg/value.h index 923327ada57..8610f917baf 100644 --- a/src/workerd/jsg/value.h +++ b/src/workerd/jsg/value.h @@ -14,6 +14,7 @@ #include #include #include +#include namespace workerd::jsg { @@ -1292,6 +1293,24 @@ class ExceptionWrapper { auto& js = Lock::from(context->GetIsolate()); auto& wrapper = TypeWrapper::from(js.v8Isolate); kj::Exception result = [&]() { + + kj::Exception::Type excType = [&]() { + if (util::Autogate::isEnabled(util::AutogateKey::ACTOR_EXCEPTION_PROPERTIES)) { + // Use .retryable and .overloaded properties as hints for what kj exception type to use. + if (handle->IsObject()) { + auto object = handle.As(); + + if (js.toBool(check(object->Get(context, v8StrIntern(js.v8Isolate, "overloaded"_kj))))) { + return kj::Exception::Type::OVERLOADED; + } + if (js.toBool(check(object->Get(context, v8StrIntern(js.v8Isolate, "retryable"_kj))))) { + return kj::Exception::Type::DISCONNECTED; + } + } + } + return kj::Exception::Type::FAILED; + }(); + KJ_IF_SOME(domException, wrapper.tryUnwrap(context, handle, (DOMException*)nullptr, parentObject)) { @@ -1336,8 +1355,7 @@ class ExceptionWrapper { reason = kj::str(JSG_EXCEPTION(Error) ": ", reason); } } - return kj::Exception(kj::Exception::Type::FAILED, __FILE__, __LINE__, - kj::mv(reason)); + return kj::Exception(excType, __FILE__, __LINE__, kj::mv(reason)); } }(); diff --git a/src/workerd/util/autogate.c++ b/src/workerd/util/autogate.c++ index cf3bfba8b7c..85f1ed819fa 100644 --- a/src/workerd/util/autogate.c++ +++ b/src/workerd/util/autogate.c++ @@ -4,6 +4,7 @@ #include "autogate.h" #include #include +#include namespace workerd::util { @@ -15,6 +16,8 @@ kj::StringPtr KJ_STRINGIFY(AutogateKey key) { return "test-workerd"_kj; case AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES: return "updated-actor-exception-types"; + case AutogateKey::ACTOR_EXCEPTION_PROPERTIES: + return "actor-exception-properties"; case AutogateKey::NumOfKeys: KJ_FAIL_ASSERT("NumOfKeys should not be used in getName"); } @@ -56,6 +59,14 @@ void Autogate::initAutogate(capnp::List::Reader gates) { globalAutogate = Autogate(gates); } +void Autogate::initEmptyAutogateForTesting() { + capnp::MallocMessageBuilder message; + auto orphanage = message.getOrphanage(); + auto gatesOrphan = orphanage.newOrphan>(0); + auto gates = gatesOrphan.get(); + initAutogate(gates.asReader()); +} + void Autogate::deinitAutogate() { globalAutogate = kj::none; } } // namespace workerd::server diff --git a/src/workerd/util/autogate.h b/src/workerd/util/autogate.h index b693a2db079..7ac8ca2b0c3 100644 --- a/src/workerd/util/autogate.h +++ b/src/workerd/util/autogate.h @@ -14,6 +14,7 @@ namespace workerd::util { enum class AutogateKey { TEST_WORKERD, UPDATED_ACTOR_EXCEPTION_TYPES, // updates exception types to better match retriability + ACTOR_EXCEPTION_PROPERTIES, // adds .retryable and .overloaded properties to tunneled exceptions NumOfKeys // Reserved for iteration. }; @@ -40,6 +41,10 @@ class Autogate { // process before any threads are created. static void initAutogate( capnp::List::Reader autogates); + + // Convenience method for tests to use to invoke initAutogate() + static void initEmptyAutogateForTesting(); + // Destroys an initialised global Autogate instance. Used only for testing. static void deinitAutogate(); private: