From 9a5a6ee4a693a0a0ae85e5f1697670a35933e769 Mon Sep 17 00:00:00 2001 From: Dan Lapid Date: Thu, 14 Nov 2024 02:15:54 +0000 Subject: [PATCH] Refactor observer to split JsgIsolateObserver and IsolateObserver This will allow for better separation of responsibilities between the two. --- src/workerd/io/observer.h | 4 +++- src/workerd/io/worker.c++ | 12 ++++++------ src/workerd/io/worker.h | 13 +++++++------ src/workerd/jsg/disable-memoize-test.c++ | 2 +- src/workerd/server/server.c++ | 9 +++++---- src/workerd/server/workerd-api.c++ | 20 ++++++-------------- src/workerd/server/workerd-api.h | 8 +++----- src/workerd/tests/test-fixture.c++ | 3 ++- 8 files changed, 33 insertions(+), 38 deletions(-) diff --git a/src/workerd/io/observer.h b/src/workerd/io/observer.h index 4668c3a09ad..8c6e741f69d 100644 --- a/src/workerd/io/observer.h +++ b/src/workerd/io/observer.h @@ -141,7 +141,9 @@ class RequestObserver: public kj::Refcounted { } }; -class IsolateObserver: public kj::AtomicRefcounted, public jsg::IsolateObserver { +class JsgIsolateObserver: public kj::AtomicRefcounted, public jsg::IsolateObserver {}; + +class IsolateObserver: public kj::AtomicRefcounted { public: virtual ~IsolateObserver() noexcept(false) {} diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index ef25deec43a..413b4ecf8fb 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -663,7 +663,7 @@ struct Worker::Isolate::Impl { // because our GlobalScope object needs to have a function called on it, and any attached // inspector needs to be notified. JSG doesn't know about these things. - Impl(Api& api, + Impl(const Api& api, IsolateObserver& metrics, IsolateLimitEnforcer& limitEnforcer, InspectorPolicy inspectorPolicy) @@ -971,20 +971,21 @@ const HeapSnapshotDeleter HeapSnapshotDeleter::INSTANCE; } // namespace Worker::Isolate::Isolate(kj::Own apiParam, + kj::Own metricsParam, kj::StringPtr id, kj::Own limitEnforcerParam, InspectorPolicy inspectorPolicy, ConsoleMode consoleMode) - : metrics(kj::atomicAddRef(apiParam->getMetrics())), + : metrics(kj::mv(metricsParam)), id(kj::str(id)), - api(kj::mv(apiParam)), limitEnforcer(kj::mv(limitEnforcerParam)), + api(kj::mv(apiParam)), consoleMode(consoleMode), featureFlagsForFl(makeCompatJson(decompileCompatibilityFlagsForFl(api->getFeatureFlags()))), impl(kj::heap(*api, *metrics, *limitEnforcer, inspectorPolicy)), weakIsolateRef(WeakIsolateRef::wrap(this)), traceAsyncContextKey(kj::refcounted()) { - api->setEnforcer(*limitEnforcer); + api->setIsolateObserver(*metrics); metrics->created(); // We just created our isolate, so we don't need to use Isolate::Impl::Lock (nor an async lock). jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) { @@ -1329,7 +1330,7 @@ Worker::Script::Script(kj::Own isolateParam, KJ_SWITCH_ONEOF(source) { KJ_CASE_ONEOF(script, ScriptSource) { impl->globals = - script.compileGlobals(lock, isolate->getApi(), isolate->impl->metrics); + script.compileGlobals(lock, isolate->getApi(), isolate->getApi().getObserver()); { // It's unclear to me if CompileUnboundScript() can get trapped in any infinite loops or @@ -1402,7 +1403,6 @@ Worker::Isolate::~Isolate() noexcept(false) { auto inspector = kj::mv(impl->inspector); auto dropTraceAsyncContextKey = kj::mv(traceAsyncContextKey); }); - api->invalidateEnforcer(); } Worker::Script::~Script() noexcept(false) { diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index 4552d0c1d9e..ab31be89224 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -274,7 +274,11 @@ class Worker::Isolate: public kj::AtomicRefcounted { // Usually it matches the script ID. An exception is preview isolates: there, each preview // session has one isolate which may load many iterations of the script (this allows the // inspector session to stay open across them). + // The Isolate object owns the Api object and outlives it in order to report teardown timing. + // The Api object is created before the Isolate object and does not strictly require + // request-specific information. explicit Isolate(kj::Own api, + kj::Own metrics, kj::StringPtr id, kj::Own limitEnforcer, InspectorPolicy inspectorPolicy, @@ -409,9 +413,8 @@ class Worker::Isolate: public kj::AtomicRefcounted { TeardownFinishedGuard teardownGuard{*metrics}; kj::String id; - kj::Own api; - // TODO: should this be before or after api? kj::Own limitEnforcer; + kj::Own api; ConsoleMode consoleMode; // If non-null, a serialized JSON object with a single "flags" property, which is a list of @@ -534,10 +537,8 @@ class Worker::Api { virtual jsg::JsObject wrapExecutionContext( jsg::Lock& lock, jsg::Ref ref) const = 0; - virtual IsolateObserver& getMetrics() = 0; - virtual const IsolateObserver& getMetrics() const = 0; - virtual void setEnforcer(IsolateLimitEnforcer&) = 0; - virtual void invalidateEnforcer() = 0; + virtual const jsg::IsolateObserver& getObserver() const = 0; + virtual void setIsolateObserver(IsolateObserver&) = 0; // Set the module fallback service callback, if any. using ModuleFallbackCallback = kj::Maybe>( diff --git a/src/workerd/jsg/disable-memoize-test.c++ b/src/workerd/jsg/disable-memoize-test.c++ index de140e08c0b..615358a9ed6 100644 --- a/src/workerd/jsg/disable-memoize-test.c++ +++ b/src/workerd/jsg/disable-memoize-test.c++ @@ -137,7 +137,7 @@ void expectEval( } KJ_TEST("Create a context with memoization disabled change flags then create another context") { - auto observer = kj::atomicRefcounted(); + auto observer = kj::atomicRefcounted(); capnp::MallocMessageBuilder flagsArena; auto flags = flagsArena.initRoot<::workerd::CompatibilityFlags>(); auto flagsReader = flags.asReader(); diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++ index 8718cd85308..75129683e19 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -2921,6 +2921,7 @@ kj::Own Server::makeWorker(kj::StringPtr name, } }; + auto jsgobserver = kj::atomicRefcounted(); auto observer = kj::atomicRefcounted(); auto limitEnforcer = kj::refcounted(); @@ -2930,19 +2931,19 @@ kj::Own Server::makeWorker(kj::StringPtr name, "The new ModuleRegistry implementation is an experimental feature. " "You must run workerd with `--experimental` to use this feature."); newModuleRegistry = WorkerdApi::initializeBundleModuleRegistry( - *observer, conf, featureFlags.asReader(), pythonConfig); + *jsgobserver, conf, featureFlags.asReader(), pythonConfig); } auto api = kj::heap(globalContext->v8System, featureFlags.asReader(), - limitEnforcer->getCreateParams(), kj::mv(observer), *memoryCacheProvider, pythonConfig, + limitEnforcer->getCreateParams(), kj::mv(jsgobserver), *memoryCacheProvider, pythonConfig, kj::mv(newModuleRegistry)); auto inspectorPolicy = Worker::Isolate::InspectorPolicy::DISALLOW; if (inspectorOverride != kj::none) { // For workerd, if the inspector is enabled, it is always fully trusted. inspectorPolicy = Worker::Isolate::InspectorPolicy::ALLOW_FULLY_TRUSTED; } - auto isolate = kj::atomicRefcounted(kj::mv(api), name, kj::mv(limitEnforcer), - inspectorPolicy, + auto isolate = kj::atomicRefcounted(kj::mv(api), kj::mv(observer), name, + kj::mv(limitEnforcer), inspectorPolicy, conf.isServiceWorkerScript() ? Worker::ConsoleMode::INSPECTOR_ONLY : consoleMode); // If we are using the inspector, we need to register the Worker::Isolate diff --git a/src/workerd/server/workerd-api.c++ b/src/workerd/server/workerd-api.c++ index b459fe71a2c..89c483e36d0 100644 --- a/src/workerd/server/workerd-api.c++ +++ b/src/workerd/server/workerd-api.c++ @@ -132,7 +132,7 @@ static const PythonConfig defaultConfig{ struct WorkerdApi::Impl final { kj::Own features; kj::Maybe> maybeOwnedModuleRegistry; - kj::Own observer; + kj::Own observer; JsgWorkerdIsolate jsgIsolate; api::MemoryCacheProvider& memoryCacheProvider; const PythonConfig& pythonConfig; @@ -160,7 +160,7 @@ struct WorkerdApi::Impl final { Impl(jsg::V8System& v8System, CompatibilityFlags::Reader featuresParam, v8::Isolate::CreateParams createParams, - kj::Own observerParam, + kj::Own observerParam, api::MemoryCacheProvider& memoryCacheProvider, const PythonConfig& pythonConfig = defaultConfig, kj::Maybe> newModuleRegistry = kj::none) @@ -169,10 +169,7 @@ struct WorkerdApi::Impl final { observer(kj::atomicAddRef(*observerParam)), jsgIsolate(v8System, Configuration(*this), kj::mv(observerParam), kj::mv(createParams)), memoryCacheProvider(memoryCacheProvider), - pythonConfig(pythonConfig) { - jsgIsolate.runInLockScope( - [&](JsgWorkerdIsolate::Lock& lock) { limitEnforcer->customizeIsolate(lock.v8Isolate); }); - } + pythonConfig(pythonConfig) {} static v8::Local compileTextGlobal( JsgWorkerdIsolate::Lock& lock, capnp::Text::Reader reader) { @@ -215,7 +212,7 @@ struct WorkerdApi::Impl final { WorkerdApi::WorkerdApi(jsg::V8System& v8System, CompatibilityFlags::Reader features, v8::Isolate::CreateParams createParams, - kj::Own observer, + kj::Own observer, api::MemoryCacheProvider& memoryCacheProvider, const PythonConfig& pythonConfig, kj::Maybe> newModuleRegistry) @@ -271,16 +268,11 @@ jsg::JsObject WorkerdApi::wrapExecutionContext( kj::downcast(lock).wrap(lock.v8Context(), kj::mv(ref))); } -IsolateObserver& WorkerdApi::getMetrics() { - return *impl->observer; -} - -const IsolateObserver& WorkerdApi::getMetrics() const { +const jsg::IsolateObserver& WorkerdApi::getObserver() const { return *impl->observer; } -void WorkerdApi::setEnforcer(IsolateLimitEnforcer&) {} -void WorkerdApi::invalidateEnforcer() {} +void WorkerdApi::setIsolateObserver(IsolateObserver&) {}; Worker::Script::Source WorkerdApi::extractSource(kj::StringPtr name, config::Worker::Reader conf, diff --git a/src/workerd/server/workerd-api.h b/src/workerd/server/workerd-api.h index d1d3f1f8de2..8fedc9ccbee 100644 --- a/src/workerd/server/workerd-api.h +++ b/src/workerd/server/workerd-api.h @@ -35,7 +35,7 @@ class WorkerdApi final: public Worker::Api { WorkerdApi(jsg::V8System& v8System, CompatibilityFlags::Reader features, v8::Isolate::CreateParams createParams, - kj::Own observer, + kj::Own observer, api::MemoryCacheProvider& memoryCacheProvider, const PythonConfig& pythonConfig, kj::Maybe> newModuleRegistry); @@ -55,10 +55,8 @@ class WorkerdApi final: public Worker::Api { jsg::Lock& lock) const override; jsg::JsObject wrapExecutionContext( jsg::Lock& lock, jsg::Ref ref) const override; - IsolateObserver& getMetrics() override; - const IsolateObserver& getMetrics() const override; - void setEnforcer(IsolateLimitEnforcer&) override; - void invalidateEnforcer() override; + const jsg::IsolateObserver& getObserver() const override; + void setIsolateObserver(IsolateObserver&) override; static Worker::Script::Source extractSource(kj::StringPtr name, config::Worker::Reader conf, diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index 755a242487b..22aad93051f 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -324,11 +324,12 @@ TestFixture::TestFixture(SetupParams&& params) api(kj::heap(testV8System, params.featureFlags.orDefault(CompatibilityFlags::Reader()), kj::heap()->getCreateParams(), - kj::atomicRefcounted(), + kj::atomicRefcounted(), *memoryCacheProvider, defaultPythonConfig, kj::none)), workerIsolate(kj::atomicRefcounted(kj::mv(api), + kj::atomicRefcounted(), scriptId, kj::heap(), Worker::Isolate::InspectorPolicy::DISALLOW)),