diff --git a/src/workerd/io/limit-enforcer.h b/src/workerd/io/limit-enforcer.h index a396420e166..3287be9b5ee 100644 --- a/src/workerd/io/limit-enforcer.h +++ b/src/workerd/io/limit-enforcer.h @@ -17,7 +17,7 @@ static constexpr size_t DEFAULT_MAX_PBKDF2_ITERATIONS = 100'000; // Interface for an object that enforces resource limits on an Isolate level. // // See also LimitEnforcer, which enforces on a per-request level. -class IsolateLimitEnforcer { +class IsolateLimitEnforcer: public kj::Refcounted { public: // Get CreateParams to pass when constructing a new isolate. virtual v8::Isolate::CreateParams getCreateParams() = 0; 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 e33d25fa3e2..6ca0c3b3a1e 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -672,6 +672,8 @@ struct Worker::Isolate::Impl { actorCacheLru(limitEnforcer.getActorCacheLruOptions()) { jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) { auto lock = api.lock(stackScope); + + limitEnforcer.customizeIsolate(lock->v8Isolate); if (inspectorPolicy != InspectorPolicy::DISALLOW) { // We just created our isolate, so we don't need to use Isolate::Impl::Lock. KJ_ASSERT(!isMultiTenantProcess(), "inspector is not safe in multi-tenant processes"); @@ -969,18 +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)), + limitEnforcer(kj::mv(limitEnforcerParam)), api(kj::mv(apiParam)), - limitEnforcer(api->getLimitEnforcer()), consoleMode(consoleMode), featureFlagsForFl(makeCompatJson(decompileCompatibilityFlagsForFl(api->getFeatureFlags()))), - impl(kj::heap(*api, *metrics, limitEnforcer, inspectorPolicy)), + impl(kj::heap(*api, *metrics, *limitEnforcer, inspectorPolicy)), weakIsolateRef(WeakIsolateRef::wrap(this)), traceAsyncContextKey(kj::refcounted()) { + 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 +1334,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 @@ -1387,7 +1392,7 @@ Worker::Isolate::~Isolate() noexcept(false) { // Update the isolate stats one last time to make sure we're accurate for cleanup in // `evicted()`. - limitEnforcer.reportMetrics(*metrics); + limitEnforcer->reportMetrics(*metrics); metrics->evicted(); weakIsolateRef->invalidate(); @@ -3808,7 +3813,7 @@ kj::Own Worker::Isolate::newScript(kj::StringPtr scriptId, } void Worker::Isolate::completedRequest() const { - limitEnforcer.completedRequest(id); + limitEnforcer->completedRequest(id); } bool Worker::Isolate::isInspectorEnabled() const { diff --git a/src/workerd/io/worker.h b/src/workerd/io/worker.h index 68e01dd5c27..ab31be89224 100644 --- a/src/workerd/io/worker.h +++ b/src/workerd/io/worker.h @@ -274,8 +274,13 @@ 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, ConsoleMode consoleMode = ConsoleMode::INSPECTOR_ONLY); @@ -305,11 +310,11 @@ class Worker::Isolate: public kj::AtomicRefcounted { kj::Maybe errorReporter = kj::none) const; inline IsolateLimitEnforcer& getLimitEnforcer() { - return limitEnforcer; + return *limitEnforcer; } inline const IsolateLimitEnforcer& getLimitEnforcer() const { - return limitEnforcer; + return *limitEnforcer; } inline Api& getApi() { @@ -408,8 +413,8 @@ class Worker::Isolate: public kj::AtomicRefcounted { TeardownFinishedGuard teardownGuard{*metrics}; kj::String id; + kj::Own limitEnforcer; kj::Own api; - IsolateLimitEnforcer& limitEnforcer; ConsoleMode consoleMode; // If non-null, a serialized JSON object with a single "flags" property, which is a list of @@ -532,10 +537,8 @@ class Worker::Api { virtual jsg::JsObject wrapExecutionContext( jsg::Lock& lock, jsg::Ref ref) const = 0; - virtual IsolateLimitEnforcer& getLimitEnforcer() = 0; - virtual const IsolateLimitEnforcer& getLimitEnforcer() const = 0; - virtual IsolateObserver& getMetrics() = 0; - virtual const IsolateObserver& getMetrics() const = 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/server/server.c++ b/src/workerd/server/server.c++ index e7ff45d15f8..75129683e19 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -2921,8 +2921,9 @@ kj::Own Server::makeWorker(kj::StringPtr name, } }; + auto jsgobserver = kj::atomicRefcounted(); auto observer = kj::atomicRefcounted(); - auto limitEnforcer = kj::heap(); + auto limitEnforcer = kj::refcounted(); kj::Maybe> newModuleRegistry; if (featureFlags.getNewModuleRegistry()) { @@ -2930,18 +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(), kj::mv(limitEnforcer), - kj::mv(observer), *memoryCacheProvider, pythonConfig, kj::mv(newModuleRegistry)); + auto api = kj::heap(globalContext->v8System, featureFlags.asReader(), + 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, 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 ceb1ca79cce..89c483e36d0 100644 --- a/src/workerd/server/workerd-api.c++ +++ b/src/workerd/server/workerd-api.c++ @@ -132,8 +132,7 @@ static const PythonConfig defaultConfig{ struct WorkerdApi::Impl final { kj::Own features; kj::Maybe> maybeOwnedModuleRegistry; - kj::Own limitEnforcer; - kj::Own observer; + kj::Own observer; JsgWorkerdIsolate jsgIsolate; api::MemoryCacheProvider& memoryCacheProvider; const PythonConfig& pythonConfig; @@ -160,24 +159,17 @@ struct WorkerdApi::Impl final { Impl(jsg::V8System& v8System, CompatibilityFlags::Reader featuresParam, - kj::Own limitEnforcerParam, - kj::Own observerParam, + v8::Isolate::CreateParams createParams, + kj::Own observerParam, api::MemoryCacheProvider& memoryCacheProvider, const PythonConfig& pythonConfig = defaultConfig, kj::Maybe> newModuleRegistry = kj::none) : features(capnp::clone(featuresParam)), maybeOwnedModuleRegistry(kj::mv(newModuleRegistry)), - limitEnforcer(kj::mv(limitEnforcerParam)), observer(kj::atomicAddRef(*observerParam)), - jsgIsolate(v8System, - Configuration(*this), - kj::mv(observerParam), - limitEnforcer->getCreateParams()), + 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) { @@ -219,14 +211,14 @@ struct WorkerdApi::Impl final { WorkerdApi::WorkerdApi(jsg::V8System& v8System, CompatibilityFlags::Reader features, - kj::Own limitEnforcer, - kj::Own observer, + v8::Isolate::CreateParams createParams, + kj::Own observer, api::MemoryCacheProvider& memoryCacheProvider, const PythonConfig& pythonConfig, kj::Maybe> newModuleRegistry) : impl(kj::heap(v8System, features, - kj::mv(limitEnforcer), + kj::mv(createParams), kj::mv(observer), memoryCacheProvider, pythonConfig, @@ -276,21 +268,11 @@ jsg::JsObject WorkerdApi::wrapExecutionContext( kj::downcast(lock).wrap(lock.v8Context(), kj::mv(ref))); } -IsolateLimitEnforcer& WorkerdApi::getLimitEnforcer() { - return *impl->limitEnforcer; -} - -const IsolateLimitEnforcer& WorkerdApi::getLimitEnforcer() const { - return *impl->limitEnforcer; -} - -IsolateObserver& WorkerdApi::getMetrics() { +const jsg::IsolateObserver& WorkerdApi::getObserver() const { return *impl->observer; } -const IsolateObserver& WorkerdApi::getMetrics() const { - return *impl->observer; -} +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 bec54bbd8a5..8fedc9ccbee 100644 --- a/src/workerd/server/workerd-api.h +++ b/src/workerd/server/workerd-api.h @@ -34,8 +34,8 @@ class WorkerdApi final: public Worker::Api { public: WorkerdApi(jsg::V8System& v8System, CompatibilityFlags::Reader features, - kj::Own limitEnforcer, - kj::Own observer, + v8::Isolate::CreateParams createParams, + 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; - IsolateLimitEnforcer& getLimitEnforcer() override; - const IsolateLimitEnforcer& getLimitEnforcer() const override; - IsolateObserver& getMetrics() override; - const IsolateObserver& getMetrics() const 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 4be9020526b..22aad93051f 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -323,13 +323,16 @@ TestFixture::TestFixture(SetupParams&& params) memoryCacheProvider(kj::heap(*timer)), api(kj::heap(testV8System, params.featureFlags.orDefault(CompatibilityFlags::Reader()), - kj::heap(), - kj::atomicRefcounted(), + kj::heap()->getCreateParams(), + kj::atomicRefcounted(), *memoryCacheProvider, defaultPythonConfig, kj::none)), - workerIsolate(kj::atomicRefcounted( - kj::mv(api), scriptId, Worker::Isolate::InspectorPolicy::DISALLOW)), + workerIsolate(kj::atomicRefcounted(kj::mv(api), + kj::atomicRefcounted(), + scriptId, + kj::heap(), + Worker::Isolate::InspectorPolicy::DISALLOW)), workerScript(kj::atomicRefcounted(kj::atomicAddRef(*workerIsolate), scriptId, server::WorkerdApi::extractSource(mainModuleName,