Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable FinalizationRegistry #3560

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Enable FinalizationRegistry #3560

wants to merge 8 commits into from

Conversation

ketanhwr
Copy link
Member

No description provided.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to make V8System entirely hidden here so that it's not exposed at all from jsg.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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd be enabling WeakRef too right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the MDN docs for WeakRef say:

If your code has just created a WeakRef for a target object, or has gotten a target object from a WeakRef's deref method, that target object will not be reclaimed until the end of the current JavaScript job (including any promise reaction jobs that run at the end of a script job). That is, you can only "see" an object get reclaimed between turns of the event loop. This is primarily to avoid making the behavior of any given JavaScript engine's garbage collector apparent in code — because if it were, people would write code relying on that behavior, which would break when the garbage collector's behavior changed. (Garbage collection is a hard problem; JavaScript engine implementers are constantly refining and improving how it works.)

Does this help with your concerns, @harrishancock?

For my part, my main concern is that we can think of how we'd implement record/replay deterministically. I think it's pretty straightforward: in record mode, we intercept WeakRef.deref() calls and make a recording that the call happened, and whether the object was still live. In replay mode, we replace WeakRef with something that isn't really a weak ref, but holds onto the object until the last recorded deref() call that was recorded as returning the live value, then drops the reference at that point and returns undefined after that.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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)) {}
Copy link
Member

@kentonv kentonv Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ketanhwr ketanhwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: tests (was testing adhoc as of now)

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

void IoContext::pumpMessageLoop() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

jsg::throwTunneledException(workerLock.getIsolate(), jsException);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -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{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ketanhwr force-pushed the ketan/enable-fr branch 4 times, most recently from da9d919 to d9a4ed0 Compare March 3, 2025 18:16
@ketanhwr ketanhwr marked this pull request as ready for review March 3, 2025 18:24
@ketanhwr ketanhwr requested review from a team as code owners March 3, 2025 18:24
@ketanhwr ketanhwr requested review from npaun, thomasgauvin and jasnell and removed request for thomasgauvin March 3, 2025 18:24
let httpPort = await workerd.getListenPort('http');
for (let i = 0; i < 3; ++i) {
const response = await fetch(`http://localhost:${httpPort}`);
assert.equal(await response.text(), i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.equal(await response.text(), i);
assert.strictEqual(await response.text(), i);

@@ -270,6 +271,11 @@ void Lock::terminateExecution() {
v8Isolate->TerminateExecution();
}

void Lock::pumpMessageLoop() {
auto platform = IsolateBase::from(v8Isolate).getDefaultPlatform();
while (v8::platform::PumpMessageLoop(platform, v8Isolate)) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be using a class method as a static method, why not use platform.PumpMessageLoop(v8Isolate)?
On that note, another Nit of mine is I would want to explicitly pass MessageLoopBehavior::kDoNotWait rather than rely on default function argument.

Another note, the while(...) is concerning. I guess we are still covered by the thread's watchdog..
Essentially the while loop is to solve for if someone registers another finalizer inside of the finalizers right?
Wonder if we should limit this to some const amount of loops.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no platform.PumpMessageLoop(v8Isolate) method as part of v8::Platform unfortunately, it's part of v8::DefaultPlatform which isn't exported.

The while(...) is to execute each "cleanup" task, not necessarily for the recursive use case. Although I should test how that behaves as well


~V8System() noexcept(false);

typedef void FatalErrorCallback(kj::StringPtr location, kj::StringPtr message);
static void setFatalErrorCallback(FatalErrorCallback* callback);

auto& getDefaultPlatform() {
Copy link
Collaborator

@danlapid danlapid Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Let's use an explicit type here please.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's better to return a pointer instead of a reference here 🤔

@ketanhwr
Copy link
Member Author

@jasnell gentle ping - whenever you get time

explicit V8System(v8::Platform& platform, kj::ArrayPtr<const kj::StringPtr> flags);
explicit V8System(v8::Platform& platform,
kj::ArrayPtr<const kj::StringPtr> flags,
v8::Platform* defaultPlatformPtr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some commentary explaining that platform is expected to be some sort of wrapper around a default platform, and you must pass in the default platform pointer so that the message loop can be pumped.

Although arguably it would be more reusable if instead the caller passed in a callback function to pump the message loop, so that the platform doesn't actually have to wrap the default platform at all.

try {
jsg::Lock& js = workerLock;
js.pumpMessageLoop();
} catch (const jsg::JsExceptionThrown&) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any way for jsg::JsExceptionThrown to be thrown by pumpMessageLoop(), so can this whole catch block be deleted?

(And also the KJ_DEFER, etc.)

@@ -447,9 +448,11 @@ kj::Promise<void> IoContext::IncomingRequest::drain() {
// For non-actor requests, apply the configured soft timeout, typically 30 seconds.
timeoutPromise = context->limitEnforcer->limitDrain();
}
context->pumpMessageLoop();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so you are pumping the message loop at drain() time. This means that it'll be pumped once per request, some time after the response is returned, but possibly before the request is totally done (if the request has waitUntil tasks).

Is this... actually right? If someone has a long-running request (and no concurrent requests), then I guess the message loop will never be pumped, so they'll never get any finalization callbacks.

Sorry, I know I already asked "is this the right place?" once before, you changed it, and now I'm asking again... I should have probably made a more concrete recommendation in the first place.

The right answer really depends on whether the message loop tasks are expected to be cheap or expensive. I'm not really clear on this, though I'd guess they are mostly cheap -- the only reason the message loop is used at all is so that the execution can be asynchronous.

If they are cheap, then the right place to pump the message loop would be right before releasing the isolate lock, every time we release the lock. This should actually be implemented in Worker::Lock, I think, not in IoContext.

If they are expensive, then we want to make sure they run asynchronously without blocking any I/O. To do that, just before releasing a lock, we would not run the loop, but check if it has any messages in it and, if so, schedule an asynchronous task to run them. This async task should probably use kj::yieldUntilWouldSleep() to defer until there's nothing else to do.

I would probably assume for now that they are cheap, and implement the first option.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it,
A key use case we need to support are DOs written in workers-rs which run out of memory.
I'm not actually sure when drain is called on those but seems right to run this more than once per request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but I guess the problem with completely separating this from IoContext is that you need to apply the CPU limiter.

But the reason you don't want to put this right next to js.runMicrotasks() in IoContext::runImpl() is because you don't want the current IoContext to be set during the finalization callbacks?

I think the right answer there might actually be to... set threadLocalRequest = nullptr early? I think you could do that, right after js.runMicrotasks(), because it's what's going to happen anyway when you return.

js.runMicrotasks();
threadLocalRequest = nullptr;
js.pumpMessageLoop();

BTW, what happens if a finalizer queues microtasks? Does pumpMessageLoop() run them or do we need another call to runMicrotasks() to make them run?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might help for the not-wanting-the-IoContext-to-be-set now that it has landed: https://github.com/cloudflare/workerd/pull/3712/files#diff-b6ab0724c7e9c7f5b57a1f51b4ebd8480e3240c7823f3e2f09e9dbc8b31ed12aR1000

{
  SuppressIoContextScope noIoScope;
  js.pumpMessageLoop();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, what happens if a finalizer queues microtasks? Does pumpMessageLoop() run them or do we need another call to runMicrotasks() to make them run?

A quick test shows that any microtasks queued during the callback are not drained by v8::platform::PumpMessageLoop

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick test shows that any microtasks queued during the callback are not drained by v8::platform::PumpMessageLoop

Yeah that's not good -- those microtasks will end up running whenever the next event is delivered. We will need to drain them, and I suppose we may then have to pump the message loop again, and so on.

Perhaps this actually belongs inside Lock::pumpMessageLoop() -- the while() loop there should have a body that runs runMicrotasks()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course meanwhile in this loop we need to be able to check whether the isolate has been terminated... this is tricky.

@@ -270,6 +271,11 @@ void Lock::terminateExecution() {
v8Isolate->TerminateExecution();
}

void Lock::pumpMessageLoop() {
auto platform = IsolateBase::from(v8Isolate).getDefaultPlatform();
while (v8::platform::PumpMessageLoop(platform, v8Isolate, v8::platform::MessageLoopBehavior::kDoNotWait)) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oy, it feels gnarly to add a while loop here. I'm assuming that we are assured enough that this won't go into an infinite loop at all? A comment here would probably be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants