Enable FinalizationRegistry #3560

ketanhwr · 2025-02-17T11:50:35Z

No description provided.

jasnell · 2025-02-18T01:15:42Z

samples/helloworld_esm/worker.js

@@ -4,6 +4,82 @@

 export default {
  async fetch(req, env) {
-    return new Response("Hello World\n");
+    let resp = await fetch("https://example.com");


Don't forget to revert this sample back to its original state when this is done. Most likely it would make sense to have this as a separate example.

This looks like more of a test. Should it be converted to a wd-test?

src/workerd/api/global-scope.c++

src/workerd/io/io-context.c++

jasnell · 2025-02-18T01:24:12Z

src/workerd/jsg/jsg.h

@@ -2276,6 +2276,8 @@ class ExternalMemoryAdjustment final {
 // Isolate<TypeWrapper>::Lock. Usually this is only done in top-level code, and the Lock is
 // passed down to everyone else from there. See setup.h for details.

+class V8System;


I don't think we should be exposing V8System like this. The intent of JSG is to abstract V8 specifics away as much as possible and this just exposes them more. If we have to expose something here, which I'm unsure about, then the functionality should be folded into a new jsg::Lock method... like js.pumpMessageLoop() so that the details of interfacing with the v8 APIS do not need to leak more out to other areas.

Yeah, I agree. js.pumpMessageLoop() was the intended plan before I sent this out for review 😅

jasnell · 2025-02-18T01:24:49Z

src/workerd/jsg/resource.h

@@ -1431,7 +1431,7 @@ class ResourceWrapper {
    // We do not allow use of WeakRef or FinalizationRegistry because they introduce
    // non-deterministic behavior.
    check(global->Delete(context, v8StrIntern(isolate, "WeakRef"_kj)));
-    check(global->Delete(context, v8StrIntern(isolate, "FinalizationRegistry"_kj)));
+    //check(global->Delete(context, v8StrIntern(isolate, "FinalizationRegistry"_kj)));


We'd be enabling WeakRef too right?

The current plan is to only enable FinalizationRegistry, given the immediate memory cleanup benefits for wasm users. WeakRef is something we can probably think about as a followup.

Is there actually anything more needed to enable WeakRef? If it's just a matter of removing the line above, I think we should do it in this change.

Yes it should work by just removing the line, although @harrishancock was concerned about WeakRef giving immediate notification of GC collection instead of FinalizationRegistry which is more non-deterministic.

WeakRef does not give immediate notification thankfully. I suppose someone could poll it as quickly as possible to approximate immediate notification but thankfully it does not provide a notification api.

My concern isn't immediate notification, but lack of control over when notifications occur. With FinalizationRegistry, we can control exactly when finalization callbacks are scheduled. With WeakRef, we have no control over when they appear empty -- that's entirely up to the GC. So, if controlling the timing of GC observation is important for risk mitigation, then enabling WeakRef is strictly higher risk than enabling FinalizationRegistry.

That said, I'd love to just accept the risk and enable WeakRef and be done with it.

I'm good with accepting the risk as I thinks it is likely quite minimal. @kentonv ?

kentonv · 2025-02-18T17:15:13Z

src/workerd/io/io-context.c++

+  if (!isCurrentNull()) {
+    KJ_LOG(ERROR, "IoContext not-null before running PumpMessageLoop()");
+  } else {
+    worker->runInLockScope(lockType, [&](Worker::Lock& lock) {


Taking a new lock here, immediately after releasing the previous one, is expensive. You should find a way to run PumpMessageLoop() just before releasing the previous lock instead.

kentonv · 2025-02-18T17:21:13Z

src/workerd/io/io-context.c++

+        jsg::Lock& js = lock;
+        auto& system = const_cast<jsg::V8System&>(js.getV8System());
+        KJ_DBG(js.v8Isolate);
+        while (v8::platform::PumpMessageLoop(&system.getDefaultPlatform(), js.v8Isolate, v8::platform::MessageLoopBehavior::kDoNotWait)) {}


It feels wrong for this code to be aware of the v8::Platform. Knowledge of how the platform is set up (and even the fact that we use the default platform) should be encapsulated inside JSG.

To that end, perhaps jsg::Lock should have a pumpMessageLoop() method, which internally calls v8::platform::PumpMessageLoop appropriately?

kentonv · 2025-02-18T17:22:44Z

src/workerd/io/io-context.c++

+        jsg::Lock& js = lock;
+        auto& system = const_cast<jsg::V8System&>(js.getV8System());
+        KJ_DBG(js.v8Isolate);
+        while (v8::platform::PumpMessageLoop(&system.getDefaultPlatform(), js.v8Isolate, v8::platform::MessageLoopBehavior::kDoNotWait)) {}


Is this actually the right time to call PumpMessageLoop -- at the end of every single ctx.run()?

Arguably it might be preferable if we invoked PumpMessageLoop asynchronously later on when we're not actively responding to a request.

I agree that we likely need to find a better place for this. We can end up running ctx.run quite a few times during a request.

kentonv · 2025-02-18T17:25:53Z

src/workerd/jsg/resource.h

@@ -1431,7 +1431,7 @@ class ResourceWrapper {
    // We do not allow use of WeakRef or FinalizationRegistry because they introduce
    // non-deterministic behavior.
    check(global->Delete(context, v8StrIntern(isolate, "WeakRef"_kj)));
-    check(global->Delete(context, v8StrIntern(isolate, "FinalizationRegistry"_kj)));
+    //check(global->Delete(context, v8StrIntern(isolate, "FinalizationRegistry"_kj)));


This probably needs a compat flag as I suspect there are people that check for the existence of these APIs and use them if they are available -- making them suddenly available would therefore cause such workers to start taking a new code path which could end up being broken.

Yeah that was something on my mind as well, had a brief discussion with @mikenomitch about placing this behind a compat date/flag.

This makes me sad, but yes, compat flag absolutely needed. I've seen such checks in the wild a few times now.

ketanhwr

TODO: tests (was testing adhoc as of now)

ketanhwr · 2025-02-26T16:12:22Z

src/workerd/io/compatibility-date.capnp

+  jsWeakRef @73 :Bool
+      $compatEnableFlag("enable_weak_ref")
+      $compatDisableFlag("disable_weak_ref");
+  # Enables WeakRefs and FinalizationRegistry API. WebAssembly based projects often rely on this API for wasm memory cleanup


Should we add a compatibility date as well (I think we should?)? How exactly do we decide what goes behind a date vs flag vs both?

ketanhwr · 2025-02-26T16:18:45Z

src/workerd/io/io-context.c++

@@ -1377,6 +1380,73 @@ kj::Promise<void> IoContext::startDeleteQueueSignalTask(IoContext* context) {
  }
 }

+void IoContext::pumpMessageLoop() {


There's a some duplication here with IoContext::runImpl(), but ensuring there's no IoContext while reusing those functions would require specializing run(), runImpl() as well as runInContextScope() to pass through a flag indicating a null IoContext. runInContextScope() also runs the code using JSG_WITHIN_CONTEXT_SCOPE which I don't think is necessarily required to pump the message loop.

ketanhwr · 2025-02-26T16:20:12Z

src/workerd/io/io-context.c++

+            workerLock.logUncaughtException(UncaughtExceptionSource::INTERNAL,
+                jsg::JsValue(jsException), jsg::JsMessage(tryCatch.Message()));
+
+            jsg::throwTunneledException(workerLock.getIsolate(), jsException);


This is copied over from runImpl() but I don't think it would be needed here since it runs after the request handler has run.. (but I don't fully understand the implications here)

ketanhwr · 2025-02-26T16:28:35Z

src/workerd/server/workerd-api.c++

@@ -271,7 +271,10 @@ struct WorkerdApi::Impl final {
        auto version = getPythonBundleName(pythonRelease);
        auto bundle = KJ_ASSERT_NONNULL(
            fetchPyodideBundle(pythonConfig, version), "Failed to get Pyodide bundle");
-        auto context = lock.newContext<api::ServiceWorkerGlobalScope>(lock.v8Isolate);
+        jsg::NewContextOptions options{


Should we enable this for python workers unconditionally or behind a flag as well? I think pyodide already has its own dummy FinalizationRegistry implementation
cc: @hoodmane

ketanhwr added 3 commits February 17, 2025 11:42

Enable Finalization Registry

3789134

PumpMessageLoop() to drain cleanup tasks

65ef124

Call PumpMessageLoop without an active IoContext

0d6cd71

jasnell reviewed Feb 18, 2025

View reviewed changes

src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved

jasnell reviewed Feb 18, 2025

View reviewed changes

src/workerd/io/io-context.c++ Outdated Show resolved Hide resolved

jasnell reviewed Feb 18, 2025

View reviewed changes

src/workerd/io/io-context.c++ Outdated Show resolved Hide resolved

jasnell reviewed Feb 18, 2025

View reviewed changes

kentonv reviewed Feb 18, 2025

View reviewed changes

ketanhwr force-pushed the ketan/enable-fr branch from 2710f33 to 777d37e Compare February 26, 2025 16:10

ketanhwr commented Feb 26, 2025

View reviewed changes

ketanhwr force-pushed the ketan/enable-fr branch from 777d37e to bb34200 Compare February 26, 2025 16:31

PumpMessageLoop() at the end of request lifecycle

69473cb

ketanhwr force-pushed the ketan/enable-fr branch from bb34200 to 69473cb Compare February 26, 2025 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable FinalizationRegistry #3560

Enable FinalizationRegistry #3560

ketanhwr commented Feb 17, 2025

jasnell Feb 18, 2025

kentonv Feb 18, 2025

jasnell Feb 18, 2025

ketanhwr Feb 18, 2025

jasnell Feb 18, 2025

ketanhwr Feb 18, 2025

kentonv Feb 18, 2025

ketanhwr Feb 18, 2025

jasnell Feb 18, 2025

harrishancock Feb 25, 2025

jasnell Feb 25, 2025

kentonv Feb 18, 2025

kentonv Feb 18, 2025

kentonv Feb 18, 2025 •

edited

Loading

jasnell Feb 18, 2025

kentonv Feb 18, 2025

ketanhwr Feb 18, 2025

jasnell Feb 18, 2025

ketanhwr left a comment

ketanhwr Feb 26, 2025

ketanhwr Feb 26, 2025

ketanhwr Feb 26, 2025

ketanhwr Feb 26, 2025

Enable FinalizationRegistry #3560

Are you sure you want to change the base?

Enable FinalizationRegistry #3560

Conversation

ketanhwr commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentonv Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentonv Feb 18, 2025 •

edited

Loading