FinalizationGroup callback should only take one holdings value.

[kmiller68]

Edit: I think we should change how the callback works instead but I'll leave this here for posterity.

We very carefully, made it so that WeakRefs could not be collected in the same event loop turn as they were constructed. We did this so that GC was less observable. It seems like we need to do the same thing for FinalizationGroup iterators otherwise the following code does the exact thing we wanted to avoid for WeakRefs.

let fg = new FinalizationGroup(function (iterator) {
    let newCount = 0;
    for (let a of iterator) {
        if (newCount !== 5) {
            fg.register({}, 1);
            newCount++;
            gc();
        }
    }
    if (newCount !== 5)
        crash();
});

fg.register({}, 2);

Maybe I'm missing something and this isn't possible though? @tschneidereit

[kmiller68]

Thinking about this more, I actually think the right API is to have the callback take one holdings value rather than an iterator. What advantages does the iteration API have over just taking the holdings value directly?

There are certainly advantages:

1. It avoids the issue above.
2. There's no reusing the iterator later bugs.
3. It seems much simpler to the user and closer to things like promises.

Even with this change, I suppose the above issue still exists with cleanupSome() but I'm concerned that API exposes far too much of the internal engine detail to users.

[mhofman]

I wasn't around when those decisions were made, but one problem I see with invoking the callback for each holding separately is that it has the potential to "spam" the program with callbacks.
In browsers, should those callbacks be purged in one go or interspersed with other work (rendering, I/O events, etc.)?
If the program's finalization work needs to be processed later, I suppose it now has to itself remember the holdings value since I assume the registration would be removed when the callback is invoked.
How would cleanupSome work? Only yield the holdings for which callback was not invoked yet? What happens when a callback for a holdings has been scheduled but the cell was unregistered or consumed through cleanupSome?

I'm not sure if indeterminism on the behavior for registering a new target inside the callback is really an issue.
Reusing of iterators can be prevented (admittedly broken right now).

[kmiller68]

I wasn't around when those decisions were made, but one problem I see with invoking the callback for each holding separately is that it has the potential to "spam" the program with callbacks.
In browsers, should those callbacks be purged in one go or interspersed with other work (rendering, I/O events, etc.)?

It seems like this problem exists for the iteration case already and I agree that it's an interesting question. Probably, it's something that browsers will want to control in order to prevent a huge GC from blocking the main run loop for too long.

If the program's finalization work needs to be processed later, I suppose it now has to itself remember the holdings value since I assume the registration would be removed when the callback is invoked.

Wouldn't this still be true with the iterator solution? If the iterator gives JS a value it's not ready to handle right now they still have to hold onto it. Or are you saying that or are you saying the JS developer would know that they don't want to handle any finalizers right now and just not run the iterator? I'm not sure that's a great API either because the JS developer doesn't know anything about the state of the system. If the system is under heavy memory pressure, the VM might want to aggressively run finalizers. There's no way for the JS developer to have the same context the VM has.

Additionally, for what it's worth, I would guess that a vast majority of JS developers will just run the iterator to completion. Thus, it's up to the VM to rate limit them anyway.

How would cleanupSome work? Only yield the holdings for which callback was not invoked yet? What happens when a callback for a holdings has been scheduled but the cell was unregistered or consumed through cleanupSome?

It seems like this is this the same as the current cleanupSome? I also think cleanupSome a dangerous API and needs to be cut but that's a discussion for a different issue.

I'm not sure if indeterminism on the behavior for registering a new target inside the callback is really an issue.
Reusing of iterators can be prevented (admittedly broken right now).

[mhofman]

Or are you saying that [...] the JS developer would know that they don't want to handle any finalizers right now and just not run the iterator?

Exactly. As a JS developer I may want to perform this finalization work at another time that suits me better.
I can just not consume the iterator and call cleanupSome later.

If the system is under heavy memory pressure, the VM might want to aggressively run finalizers. There's no way for the JS developer to have the same context the VM has.

The engine has no guarantee the JS finalizer will ever actually release some resources. At best, the engine is hoping the program's finalizers will help.

Additionally, for what it's worth, I would guess that a vast majority of JS developers will just run the iterator to completion. Thus, it's up to the VM to rate limit them anyway.

I'm not sure that's something the engine should do. And unless it makes sure to not clear WeakRefs, WeakMaps and WeakSets for the same target, it's not allowed to withhold holdings for cleared targets (at least in cleanupSome's iterator).

[kmiller68]

Or are you saying that [...] the JS developer would know that they don't want to handle any finalizers right now and just not run the iterator?

Exactly. As a JS developer I may want to perform this finalization work at another time that suits me better.
I can just not consume the iterator and call cleanupSome later.

But the engine can just immediately call into your callback again, if I understand the spec correctly.

If the system is under heavy memory pressure, the VM might want to aggressively run finalizers. There's no way for the JS developer to have the same context the VM has.

The engine has no guarantee the JS finalizer will ever actually release some resources. At best, the engine is hoping the program's finalizers will help.

True, but once the VM calls into the finalizer the VM no longer needs to retain information about the holdings value internally. So at the very least, the VM is freeing that memory.

Additionally, for what it's worth, I would guess that a vast majority of JS developers will just run the iterator to completion. Thus, it's up to the VM to rate limit them anyway.

I'm not sure that's something the engine should do. And unless it makes sure to not clear WeakRefs, WeakMaps and WeakSets for the same target, it's not allowed to withhold holdings for cleared targets (at least in cleanupSome's iterator).

Yikes! This is once again another reason I think cleanupSome() is a bad API.

[mhofman]

I can just not consume the iterator and call cleanupSome later.

But the engine can just immediately call into your callback again, if I understand the spec correctly.

Sure, and as a developer I can keep ignoring it until I'm ready to consume the iterator.

The engine has no guarantee the JS finalizer will ever actually release some resources. At best, the engine is hoping the program's finalizers will help.

True, but once the VM calls into the finalizer the VM no longer needs to retain information about the holdings value internally. So at the very least, the VM is freeing that memory.

Only if the program doesn't reference the holdings value somewhere itself (potentially from the callback code).

Additionally, for what it's worth, I would guess that a vast majority of JS developers will just run the iterator to completion. Thus, it's up to the VM to rate limit them anyway.

I'm not sure that's something the engine should do. And unless it makes sure to not clear WeakRefs, WeakMaps and WeakSets for the same target, it's not allowed to withhold holdings for cleared targets (at least in cleanupSome's iterator).

Yikes! This is once again another reason I think cleanupSome() is a bad API.

From what I understand, a synchronous API for FinalizationGroup seem to be required for WASM type use cases that don't rely on the run-loop (i.e. constructor's callback would never actually be called on its own). See #11

As for the atomicity, I wrote some thoughts on your issue in #157

[wingo]

Another alternative, taking an array of holdings instead of an iterator: #60.

[littledan]

An iterator gives a little more power to frameworks to control their frame budget (e.g., to give up even sooner than the browser would, to leave time for other sorts of computation). However, I am sympathetic to the idea that developers might just go through the array and not do this fancy scheduling.

[hotsphink]

To me, it seems like actual users who care about frame budgets would not use this additional power, and would likely want to immediately flatten the iterator into an Array to give the user code full control using its application-specific knowledge. #60 (comment)

As for a single holdings per callback vs an Array, my current position is that the Array ends up being simpler to use for an application that is managing its frame budget carefully -- with the holdings, the app would need to accumulate all holdings before processing anyway, and that's harder to do if the engine might only give it a subset of the callbacks and then it loses its chance to do the global scheduling.

To be more explicit about my usage assumptions: I think that naive applications will process all of the holdings in one go using whatever API produces them, and the specifics don't matter much except insofar as simplicity is better, especially when there are objects floating around (iterators, FinalizationGroups themselves, etc.) whose identity is observable and whose liveness must be considered. More complex applications have knowledge of their cleanup behavior that the JS engine lacks, and so will queue up all holdings/callbacks (collectively "finalizers") and schedule them with a combination of frame budgets, elapsed time measurements, and embedding facilities like requestIdleCallback. If that is the case, then providing scheduling facilities is a waste of effort. I could very well be wrong.

Admittedly, I haven't thought about cleanupSome. So far, it just feels like a footgun.