Skip to content

Commit

Permalink
Revert "Revert "Add .retryable and .overloaded properties to tunneled…
Browse files Browse the repository at this point in the history
… exception objects""

This reverts commit 051dbfc.
  • Loading branch information
jclee committed Jun 4, 2024
1 parent 1f38aee commit 37e4a9b
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 7 deletions.
2 changes: 2 additions & 0 deletions src/workerd/api/streams/standard-test.c++
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <workerd/jsg/jsg.h>
#include <workerd/jsg/jsg-test.h>
#include <workerd/jsg/observer.h>
#include <workerd/util/autogate.h>

namespace workerd::api {
namespace {
Expand All @@ -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<jsg::IsolateObserver>());
isolate.runInLockScope([&](RsIsolate::Lock& lock) {
JSG_WITHIN_CONTEXT_SCOPE(lock,
Expand Down
1 change: 1 addition & 0 deletions src/workerd/jsg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
5 changes: 4 additions & 1 deletion src/workerd/jsg/jsg-test.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "jsg.h"
#include "setup.h"
#include <kj/test.h>
#include <workerd/util/autogate.h>

namespace workerd::jsg::test {

Expand All @@ -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):
Expand Down
2 changes: 2 additions & 0 deletions src/workerd/jsg/modules-new-test.c++
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <workerd/jsg/setup.h>
#include <workerd/jsg/modules-new.h>
#include <workerd/jsg/modules.capnp.h>
#include <workerd/util/autogate.h>
#include "observer.h"
#include "url.h"
#include <kj/async-io.h>
Expand Down Expand Up @@ -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<IsolateObserver>()); \
runInV8Stack([&](auto& stackScope) { \
TestIsolate::Lock lock(isolate, stackScope); \
Expand Down
40 changes: 36 additions & 4 deletions src/workerd/jsg/util.c++
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,26 @@ bool setRemoteError(v8::Isolate* isolate, v8::Local<v8::Value>& exception) {
v8::True(isolate)));
}

bool setRetryableError(v8::Isolate* isolate, v8::Local<v8::Value>& exception) {
KJ_ASSERT(exception->IsObject());
auto obj = exception.As<v8::Object>();
return jsg::check(
obj->Set(
isolate->GetCurrentContext(),
jsg::v8StrIntern(isolate, "retryable"_kj),
v8::True(isolate)));
}

bool setOverloadedError(v8::Isolate* isolate, v8::Local<v8::Value>& exception) {
KJ_ASSERT(exception->IsObject());
auto obj = exception.As<v8::Object>();
return jsg::check(
obj->Set(
isolate->GetCurrentContext(),
jsg::v8StrIntern(isolate, "overloaded"_kj),
v8::True(isolate)));
}

bool setDurableObjectResetError(v8::Isolate* isolate, v8::Local<v8::Value>& exception) {
KJ_ASSERT(exception->IsObject());
auto obj = exception.As<v8::Object>();
Expand All @@ -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
Expand Down Expand Up @@ -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);
}
Expand All @@ -222,8 +251,6 @@ kj::StringPtr extractTunneledExceptionDescription(kj::StringPtr message) {
}
}



v8::Local<v8::Value> makeInternalError(v8::Isolate* isolate, kj::Exception&& exception) {
auto desc = exception.getDescription();

Expand All @@ -235,7 +262,7 @@ v8::Local<v8::Value> 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();
Expand All @@ -259,6 +286,11 @@ v8::Local<v8::Value> 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);
}
Expand Down
22 changes: 20 additions & 2 deletions src/workerd/jsg/value.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <kj/time.h>
#include <kj/debug.h>
#include <kj/one-of.h>
#include <workerd/util/autogate.h>

namespace workerd::jsg {

Expand Down Expand Up @@ -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<v8::Object>();

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)) {
Expand Down Expand Up @@ -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));
}
}();

Expand Down
11 changes: 11 additions & 0 deletions src/workerd/util/autogate.c++
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "autogate.h"
#include <workerd/util/sentry.h>
#include <kj/common.h>
#include <capnp/message.h>

namespace workerd::util {

Expand All @@ -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");
}
Expand Down Expand Up @@ -56,6 +59,14 @@ void Autogate::initAutogate(capnp::List<capnp::Text>::Reader gates) {
globalAutogate = Autogate(gates);
}

void Autogate::initEmptyAutogateForTesting() {
capnp::MallocMessageBuilder message;
auto orphanage = message.getOrphanage();
auto gatesOrphan = orphanage.newOrphan<capnp::List<capnp::Text>>(0);
auto gates = gatesOrphan.get();
initAutogate(gates.asReader());
}

void Autogate::deinitAutogate() { globalAutogate = kj::none; }

} // namespace workerd::server
5 changes: 5 additions & 0 deletions src/workerd/util/autogate.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
};

Expand All @@ -40,6 +41,10 @@ class Autogate {
// process before any threads are created.
static void initAutogate(
capnp::List<capnp::Text>::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:
Expand Down

0 comments on commit 37e4a9b

Please sign in to comment.