diff --git a/src/workerd/jsg/BUILD.bazel b/src/workerd/jsg/BUILD.bazel index db5e828dfd3..d713c28bd75 100644 --- a/src/workerd/jsg/BUILD.bazel +++ b/src/workerd/jsg/BUILD.bazel @@ -179,6 +179,7 @@ wd_cc_library( "resource-test.c++", "rtti-test.c++", "url-test.c++", + "disable-memoize-test.c++", ], )] @@ -229,3 +230,11 @@ kj_test( ":resource_test_capnp", ], ) + +kj_test( + src = "disable-memoize-test.c++", + deps = [ + "//src/workerd/io", + "//src/workerd/jsg", + ], +) diff --git a/src/workerd/jsg/disable-memoize-test.c++ b/src/workerd/jsg/disable-memoize-test.c++ new file mode 100644 index 00000000000..fe1750b2c95 --- /dev/null +++ b/src/workerd/jsg/disable-memoize-test.c++ @@ -0,0 +1,183 @@ +#include +#include +#include +#include + +#include +#include + +namespace workerd::jsg::test { +jsg::V8System v8System; + +struct TestApi1: public jsg::Object { + TestApi1() = default; + TestApi1(jsg::Lock&, const jsg::Url&) {} + int test1(jsg::Lock& js) { + return 1; + } + + int test2(jsg::Lock& js) { + return 2; + } + static jsg::Ref constructor() { + return jsg::alloc(); + } + + JSG_RESOURCE_TYPE(TestApi1, workerd::CompatibilityFlags::Reader flags) { + if (flags.getPythonWorkers()) { + JSG_METHOD(test2); + } else { + JSG_METHOD(test1); + } + } +}; +struct TestApi2: public jsg::Object { + TestApi2() = default; + TestApi2(jsg::Lock&, const jsg::Url&) {} + int test1(jsg::Lock& js) { + return 1; + } + + int test2(jsg::Lock& js) { + return 2; + } + static jsg::Ref constructor() { + return jsg::alloc(); + } + + JSG_RESOURCE_TYPE(TestApi2, workerd::CompatibilityFlags::Reader flags) { + if (flags.getPythonWorkers()) { + JSG_METHOD(test2); + } else { + JSG_METHOD(test1); + } + } +}; + +struct BaseTestContext: public jsg::Object, public jsg::ContextGlobal { + int test1(jsg::Lock& js) { + return 1; + } + + int test2(jsg::Lock& js) { + return 2; + } + JSG_RESOURCE_TYPE(BaseTestContext, workerd::CompatibilityFlags::Reader flags) { + if (flags.getPythonWorkers()) { + JSG_METHOD(test2); + } else { + JSG_METHOD(test1); + } + JSG_NESTED_TYPE(TestApi1); + } +}; + +struct TestContext: public BaseTestContext { + int test3(jsg::Lock& js) { + return 3; + } + + int test4(jsg::Lock& js) { + return 4; + } + JSG_RESOURCE_TYPE(TestContext, workerd::CompatibilityFlags::Reader flags) { + JSG_INHERIT(BaseTestContext); + if (flags.getPythonWorkers()) { + JSG_METHOD(test4); + } else { + JSG_METHOD(test3); + } + JSG_NESTED_TYPE(TestApi2); + } +}; + +JSG_DECLARE_ISOLATE_TYPE(TestIsolate, TestContext, BaseTestContext, TestApi1, TestApi2); + +class Configuration { +public: + Configuration(workerd::CompatibilityFlags::Reader& flags): flags(flags) {} + operator const workerd::CompatibilityFlags::Reader() const { + return flags; + } + +private: + workerd::CompatibilityFlags::Reader& flags; +}; + +void expectEval( + jsg::Lock& js, kj::StringPtr code, kj::StringPtr expectedType, kj::StringPtr expectedValue) { + // Create a string containing the JavaScript source code. + v8::Local source = jsg::v8Str(js.v8Isolate, code); + + // Compile the source code. + v8::Local script; + if (!v8::Script::Compile(js.v8Context(), source).ToLocal(&script)) { + KJ_FAIL_EXPECT("code didn't parse", code); + return; + } + + v8::TryCatch catcher(js.v8Isolate); + + // Run the script to get the result. + v8::Local result; + if (script->Run(js.v8Context()).ToLocal(&result)) { + v8::String::Utf8Value type(js.v8Isolate, result->TypeOf(js.v8Isolate)); + v8::String::Utf8Value value(js.v8Isolate, result); + + KJ_EXPECT(*type == expectedType, *type, expectedType); + KJ_EXPECT(*value == expectedValue, *value, expectedValue); + } else if (catcher.HasCaught()) { + v8::String::Utf8Value message(js.v8Isolate, catcher.Exception()); + + KJ_EXPECT(expectedType == "throws", expectedType, catcher.Exception()); + KJ_EXPECT(*message == expectedValue, *message, expectedValue); + } else { + KJ_FAIL_EXPECT("returned empty handle but didn't throw exception?"); + } +} + +KJ_TEST("Create a context with memoization disabled change flags then create another context") { + auto observer = kj::atomicRefcounted(); + capnp::MallocMessageBuilder flagsArena; + auto flags = flagsArena.initRoot<::workerd::CompatibilityFlags>(); + auto flagsReader = flags.asReader(); + Configuration config(flagsReader); + TestIsolate isolate(v8System, config, kj::atomicAddRef(*observer)); + isolate.runInLockScope([&](TestIsolate::Lock& lock) { + jsg::JsContext context = lock.newContext(); + v8::Local ctx = context.getHandle(lock); + KJ_ASSERT(!ctx.IsEmpty(), "unable to enter invalid v8::Context"); + v8::Context::Scope scope(ctx); + + expectEval(lock, "test1()", "number", "1"); + expectEval(lock, "test2()", "throws", "ReferenceError: test2 is not defined"); + expectEval(lock, "test3()", "number", "3"); + expectEval(lock, "test4()", "throws", "ReferenceError: test4 is not defined"); + expectEval(lock, "new TestApi1().test1()", "number", "1"); + expectEval(lock, "new TestApi1().test2()", "throws", + "TypeError: (intermediate value).test2 is not a function"); + expectEval(lock, "new TestApi2().test1()", "number", "1"); + expectEval(lock, "new TestApi2().test2()", "throws", + "TypeError: (intermediate value).test2 is not a function"); + }); + flags.setPythonWorkers(true); + isolate.runInLockScope([&](TestIsolate::Lock& lock) { + jsg::JsContext context = lock.newContext(); + v8::Local ctx = context.getHandle(lock); + KJ_ASSERT(!ctx.IsEmpty(), "unable to enter invalid v8::Context"); + v8::Context::Scope scope(ctx); + + expectEval(lock, "test1()", "throws", "ReferenceError: test1 is not defined"); + expectEval(lock, "test2()", "number", "2"); + expectEval(lock, "test3()", "throws", "ReferenceError: test3 is not defined"); + expectEval(lock, "test4()", "number", "4"); + expectEval(lock, "new TestApi1().test1()", "throws", + "TypeError: (intermediate value).test1 is not a function"); + expectEval(lock, "new TestApi1().test2()", "number", "2"); + expectEval(lock, "new TestApi2().test1()", "throws", + "TypeError: (intermediate value).test1 is not a function"); + expectEval(lock, "new TestApi2().test2()", "number", "2"); + }); +} + +} // namespace workerd::jsg::test diff --git a/src/workerd/jsg/resource.h b/src/workerd/jsg/resource.h index 97ab96fb7e0..9896e0e949c 100644 --- a/src/workerd/jsg/resource.h +++ b/src/workerd/jsg/resource.h @@ -458,14 +458,15 @@ template + bool isContext, + bool memoize> struct GetterCallback; #define JSG_DEFINE_GETTER_CALLBACK_STRUCTS(...) \ template \ + typename... Args, Ret (T::*method)(Args...) __VA_ARGS__, bool isContext, bool memoize> \ struct GetterCallback { \ + isContext, memoize> { \ static constexpr bool enumerable = true; \ static void callback(v8::Local, const v8::PropertyCallbackInfo& info) { \ liftKj(info, [&]() { \ @@ -474,7 +475,9 @@ struct GetterCallback; auto obj = info.This(); \ auto& wrapper = TypeWrapper::from(isolate); \ /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \ - if (!isContext && !wrapper.getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \ + if (!isContext && \ + !wrapper.template getTemplate(isolate, (T*)nullptr) \ + ->HasInstance(obj)) { \ throwTypeError(isolate, kIllegalInvocation); \ } \ auto& self = extractInternalPointer(context, obj); \ @@ -486,9 +489,10 @@ struct GetterCallback; \ /* Specialization for methods that take `Lock&` as their first parameter. */ \ template \ + typename... Args, Ret (T::*method)(Lock&, Args...) __VA_ARGS__, bool isContext, \ + bool memoize> \ struct GetterCallback { \ + isContext, memoize> { \ static constexpr bool enumerable = true; \ static void callback(v8::Local, const v8::PropertyCallbackInfo& info) { \ liftKj(info, [&]() { \ @@ -497,7 +501,9 @@ struct GetterCallback; auto obj = info.This(); \ auto& wrapper = TypeWrapper::from(isolate); \ /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \ - if (!isContext && !wrapper.getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \ + if (!isContext && \ + !wrapper.template getTemplate(isolate, (T*)nullptr) \ + ->HasInstance(obj)) { \ throwTypeError(isolate, kIllegalInvocation); \ } \ auto& self = extractInternalPointer(context, obj); \ @@ -509,9 +515,9 @@ struct GetterCallback; }; \ \ template \ + Unimplemented (T::*method)() __VA_ARGS__, bool isContext, bool memoize> \ struct GetterCallback { \ + isContext, memoize> { \ static constexpr bool enumerable = false; \ static void callback(v8::Local, const v8::PropertyCallbackInfo& info) { \ scheduleUnimplementedPropertyError(info.GetIsolate(), typeid(T), propertyName); \ @@ -527,14 +533,15 @@ template + bool isContext, + bool memoize> struct PropertyGetterCallback; #define JSG_DEFINE_PROPERTY_GETTER_CALLBACK_STRUCTS(...) \ template \ + typename... Args, Ret (T::*method)(Args...) __VA_ARGS__, bool isContext, bool memoize> \ struct PropertyGetterCallback { \ + isContext, memoize> { \ static constexpr bool enumerable = true; \ static void callback(const v8::FunctionCallbackInfo& info) { \ liftKj(info, [&]() { \ @@ -543,7 +550,9 @@ struct PropertyGetterCallback; auto obj = info.This(); \ auto& wrapper = TypeWrapper::from(isolate); \ /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \ - if (!isContext && !wrapper.getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \ + if (!isContext && \ + !wrapper.template getTemplate(isolate, (T*)nullptr) \ + ->HasInstance(obj)) { \ throwTypeError(isolate, kIllegalInvocation); \ } \ auto& self = extractInternalPointer(context, obj); \ @@ -555,9 +564,10 @@ struct PropertyGetterCallback; \ /* Specialization for methods that take `Lock&` as their first parameter. */ \ template \ + typename... Args, Ret (T::*method)(Lock&, Args...) __VA_ARGS__, bool isContext, \ + bool memoize> \ struct PropertyGetterCallback { \ + method, isContext, memoize> { \ static constexpr bool enumerable = true; \ static void callback(const v8::FunctionCallbackInfo& info) { \ liftKj(info, [&]() { \ @@ -566,7 +576,9 @@ struct PropertyGetterCallback; auto obj = info.This(); \ auto& wrapper = TypeWrapper::from(isolate); \ /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \ - if (!isContext && !wrapper.getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \ + if (!isContext && \ + !wrapper.template getTemplate(isolate, (T*)nullptr) \ + ->HasInstance(obj)) { \ throwTypeError(isolate, kIllegalInvocation); \ } \ auto& self = extractInternalPointer(context, obj); \ @@ -577,9 +589,9 @@ struct PropertyGetterCallback; } \ }; \ template \ + Unimplemented (T::*method)() __VA_ARGS__, bool isContext, bool memoize> \ struct PropertyGetterCallback { \ + method, isContext, memoize> { \ static constexpr bool enumerable = false; \ static void callback(const v8::FunctionCallbackInfo& info) { \ scheduleUnimplementedPropertyError(info.GetIsolate(), typeid(T), propertyName); \ @@ -594,7 +606,8 @@ template + bool isContext, + bool memoize> struct SetterCallback; template -struct SetterCallback { + bool isContext, + bool memoize> +struct SetterCallback { static void callback( v8::Local, v8::Local value, const v8::PropertyCallbackInfo& info) { liftKj(info, [&]() { @@ -612,7 +626,9 @@ struct SetterCallbackHasInstance(obj)) { + if (!isContext && + !wrapper.template getTemplate(isolate, (T*)nullptr) + ->HasInstance(obj)) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -628,8 +644,14 @@ template -struct SetterCallback { + bool isContext, + bool memoize> +struct SetterCallback { static void callback( v8::Local, v8::Local value, const v8::PropertyCallbackInfo& info) { liftKj(info, [&]() { @@ -638,7 +660,9 @@ struct SetterCallbackHasInstance(obj)) { + if (!isContext && + !wrapper.template getTemplate(isolate, (T*)nullptr) + ->HasInstance(obj)) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -653,7 +677,8 @@ template + bool isContext, + bool memoize> struct PropertySetterCallback; template -struct PropertySetterCallback { + bool isContext, + bool memoize> +struct PropertySetterCallback { static void callback(const v8::FunctionCallbackInfo& info) { liftKj(info, [&]() { auto isolate = info.GetIsolate(); @@ -670,7 +701,9 @@ struct PropertySetterCallbackHasInstance(obj)) { + if (!isContext && + !wrapper.template getTemplate(isolate, (T*)nullptr) + ->HasInstance(obj)) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -686,8 +719,14 @@ template -struct PropertySetterCallback { + bool isContext, + bool memoize> +struct PropertySetterCallback { static void callback(const v8::FunctionCallbackInfo& info) { liftKj(info, [&]() { auto isolate = info.GetIsolate(); @@ -695,7 +734,9 @@ struct PropertySetterCallbackHasInstance(obj)) { + if (!isContext && + !wrapper.template getTemplate(isolate, (T*)nullptr) + ->HasInstance(obj)) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -844,18 +885,24 @@ class DynamicResourceTypeMap { class JsValue; -template +template struct WildcardPropertyCallbacks; template (U::*getNamedMethod)(jsg::Lock&, kj::String)> + kj::Maybe (U::*getNamedMethod)(jsg::Lock&, kj::String), + bool memoize> struct WildcardPropertyCallbacks (U::*)(jsg::Lock&, kj::String), - getNamedMethod>: public v8::NamedPropertyHandlerConfiguration { + getNamedMethod, + memoize>: public v8::NamedPropertyHandlerConfiguration { WildcardPropertyCallbacks() : v8::NamedPropertyHandlerConfiguration(getter, nullptr, @@ -878,7 +925,7 @@ struct WildcardPropertyCallbacksGetCurrentContext(); auto obj = info.This(); auto& wrapper = TypeWrapper::from(isolate); - if (!wrapper.getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { + if (!wrapper.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { throwTypeError(isolate, kIllegalInvocation); } auto& self = extractInternalPointer(context, obj); @@ -898,7 +945,7 @@ struct WildcardPropertyCallbacks +template struct ResourceTypeBuilder { ResourceTypeBuilder(TypeWrapper& typeWrapper, v8::Isolate* isolate, @@ -927,12 +974,13 @@ struct ResourceTypeBuilder { template inline void registerWildcardProperty() { prototype->SetHandler( - WildcardPropertyCallbacks{}); + WildcardPropertyCallbacks{}); } template inline void registerInherit() { - constructor->Inherit(typeWrapper.template getTemplate(isolate, (Type*)nullptr)); + constructor->Inherit( + typeWrapper.template getTemplate(isolate, (Type*)nullptr)); } template @@ -979,7 +1027,7 @@ struct ResourceTypeBuilder { inline void registerInstanceProperty() { auto v8Name = v8StrIntern(isolate, name); - using Gcb = GetterCallback; + using Gcb = GetterCallback; if (!Gcb::enumerable) { // Mark as unimplemented if `Gcb::enumerable` is `false`. This is only the case when `Getter` // returns `Unimplemented`. @@ -987,7 +1035,7 @@ struct ResourceTypeBuilder { } instance->SetNativeDataProperty(v8Name, Gcb::callback, - &SetterCallback::callback, + &SetterCallback::callback, v8::Local(), Gcb::enumerable ? v8::PropertyAttribute::None : v8::PropertyAttribute::DontEnum); } @@ -996,7 +1044,7 @@ struct ResourceTypeBuilder { inline void registerPrototypeProperty() { auto v8Name = v8StrIntern(isolate, name); - using Gcb = PropertyGetterCallback; + using Gcb = PropertyGetterCallback; if (!Gcb::enumerable) { inspectProperties->Set(v8Name, v8::False(isolate), v8::PropertyAttribute::ReadOnly); } @@ -1006,8 +1054,8 @@ struct ResourceTypeBuilder { // to using setters... which is annoying. It means we end up having to use FunctionTemplates // instead of the more convenient property callbacks. auto getterFn = v8::FunctionTemplate::New(isolate, Gcb::callback); - auto setterFn = v8::FunctionTemplate::New( - isolate, &PropertySetterCallback::callback); + auto setterFn = v8::FunctionTemplate::New(isolate, + &PropertySetterCallback::callback); prototype->SetAccessorProperty(v8Name, getterFn, setterFn, Gcb::enumerable ? v8::PropertyAttribute::None : v8::PropertyAttribute::DontEnum); @@ -1017,7 +1065,7 @@ struct ResourceTypeBuilder { inline void registerReadonlyInstanceProperty() { auto v8Name = v8StrIntern(isolate, name); - using Gcb = GetterCallback; + using Gcb = GetterCallback; if (!Gcb::enumerable) { inspectProperties->Set(v8Name, v8::False(isolate), v8::PropertyAttribute::ReadOnly); } @@ -1039,7 +1087,7 @@ struct ResourceTypeBuilder { inline void registerReadonlyPrototypeProperty() { auto v8Name = v8StrIntern(isolate, name); - using Gcb = PropertyGetterCallback; + using Gcb = PropertyGetterCallback; if (!Gcb::enumerable) { inspectProperties->Set(v8Name, v8::False(isolate), v8::PropertyAttribute::ReadOnly); } @@ -1055,7 +1103,7 @@ struct ResourceTypeBuilder { inline void registerLazyInstanceProperty() { auto v8Name = v8StrIntern(isolate, name); - using Gcb = GetterCallback; + using Gcb = GetterCallback; if (!Gcb::enumerable) { inspectProperties->Set(v8Name, v8::False(isolate), v8::PropertyAttribute::ReadOnly); } @@ -1073,7 +1121,7 @@ struct ResourceTypeBuilder { template inline void registerInspectProperty() { - using Gcb = GetterCallback; + using Gcb = GetterCallback; auto v8Name = v8StrIntern(isolate, name); @@ -1149,7 +1197,8 @@ struct ResourceTypeBuilder { static_assert( hasGetTemplate, "Type must be listed in JSG_DECLARE_ISOLATE_TYPE to be declared nested."); - prototype->Set(isolate, name, typeWrapper.getTemplate(isolate, (Type*)nullptr)); + prototype->Set( + isolate, name, typeWrapper.template getTemplate(isolate, (Type*)nullptr)); } inline void registerTypeScriptRoot() { /* only needed for RTTI */ } @@ -1393,7 +1442,7 @@ class ResourceWrapper { } } - template + template JsContext newContext(jsg::Lock& js, jsg::NewContextOptions options, CompilationObserver& compilationObserver, @@ -1411,9 +1460,17 @@ class ResourceWrapper { // type as the global, we instantiate this separate branch specifically for that type. // Fortunately, for types that are never used as the global object, we never have to // instantiate the `isContext = true` branch. + // + // When creating a new context the global context template will be memoized for future reuse in + // additional contexts. The global context can depend on `configuration`. For Python Isolate + // Pools we want to create a new context and initialize some code in it and later when a request + // arrives, edit the configuration and create a new context. To achieve this we create the first + // context with `memoize=false` to disable memoization of the global context created for the + // first context. Then we create a new context with `memoize=true` to enable memoization of the + // main context created after a request was received. auto isolate = js.v8Isolate; - auto tmpl = getTemplate(isolate, nullptr)->InstanceTemplate(); + auto tmpl = getTemplate(isolate, nullptr)->InstanceTemplate(); v8::Local context = v8::Context::New(isolate, nullptr, tmpl); auto global = context->Global(); @@ -1492,12 +1549,14 @@ class ResourceWrapper { } } - template + template v8::Local getTemplate(v8::Isolate* isolate, T*) { v8::Global& slot = isContext ? contextConstructor : memoizedConstructor; if (slot.IsEmpty()) { - auto result = makeConstructor(isolate); - slot.Reset(isolate, result); + auto result = makeConstructor(isolate); + if constexpr (memoize) { + slot.Reset(isolate, result); + } return result; } else { return slot.Get(isolate); @@ -1509,7 +1568,7 @@ class ResourceWrapper { v8::Global memoizedConstructor; v8::Global contextConstructor; - template + template v8::Local makeConstructor(v8::Isolate* isolate) { // Construct lazily. v8::EscapableHandleScope scope(isolate); @@ -1558,7 +1617,7 @@ class ResourceWrapper { auto& typeWrapper = static_cast(*this); - ResourceTypeBuilder builder( + ResourceTypeBuilder builder( typeWrapper, isolate, constructor, instance, prototype, signature); if constexpr (isDetected()) { diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index 357d084ae3c..bdcde5bfc47 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -570,21 +570,21 @@ class Isolate: public IsolateBase { // Creates a new JavaScript "context", i.e. the global object. This is the first step to // executing JavaScript code. T should be one of your API types which you want to use as the // global object. `args...` are passed to the type's constructor. - template + template JsContext newContext(NewContextOptions options, Args&&... args) { // TODO(soon): Requiring move semantics for the global object is awkward. This should instead // allocate the object (forwarding arguments to the constructor) and return something like // a Ref. - return jsgIsolate.wrapper->newContext(*this, options, jsgIsolate.getObserver(), - static_cast(nullptr), kj::fwd(args)...); + return jsgIsolate.wrapper->template newContext(*this, options, + jsgIsolate.getObserver(), static_cast(nullptr), kj::fwd(args)...); } // Creates a new JavaScript "context", i.e. the global object. This is the first step to // executing JavaScript code. T should be one of your API types which you want to use as the // global object. `args...` are passed to the type's constructor. - template + template JsContext newContext(Args&&... args) { - return newContext(NewContextOptions{}, kj::fwd(args)...); + return newContext(NewContextOptions{}, kj::fwd(args)...); } void reportError(const JsValue& value) override {