Normative: Replace finalization iterator with multiple callback calls #187

littledan · 2020-02-21T15:59:34Z

Note, there's no microtask checkpoint between all these callbacks--it's like Array.prototype.map

Landing this is blocking on some sort of consensus that it's the right way. Discussion is ongoing in #60.

Closes #155
Closes #60

README.md

mhofman · 2020-02-22T00:37:33Z

README.md


-The `FinalizationGroup` callback is passed an iterator of held values to give that callback control over how much work it wants to process. The callback may pull in only part of the iterator, and in this case, the rest of the work would be "saved for later". The callback is not called during execution of other JavaScript code, but rather "in between turns"; it is currently proposed to be restricted to run after all of the `Promise`-related work is done, right before turning control over to the event loop.
+The `FinalizationRegistry` callback is called potentially multiple times, for each registered object that becomes dead, with a relevant held value. The callback is not called during execution of other JavaScript code, but rather "in between turns"--it is not interspersed with Promise work, for example, but only runs after all of the Promises have been processed.


Interestingly the Promise interspersing would be one way for a program to detect that a callback is part of a "batch" of empties cells. A program probably shouldn't rely on non-interspersing since an engine is free to empty cells one at a time (or make it look like it did)

Right, non-interspersing of the finalization is not a reliable property (maybe I should just not mention it?), but it is reliable that it won't intersperse into Promises which are queued before the finalizers more (at least on the web and environments that queue tasks in a similar way).

It's probably a valid warning to say that Promise work may not be performed before the next callback invocation and that a program should be careful if it does any asynchronous finalization work?

I'm just worried about making it sound like Promise work will never be performed between callback invocations since engines are free to stop iterations whenever they want.

Thanks for raising this concern. Does @syg's wording resolve the issue for you?

mhofman · 2020-02-22T00:39:49Z

spec/abstract-jobs.html

  <emu-clause id="sec-cleanup-finalization-registry" aoid="CleanupFinalizationRegistry">
    <h1> CleanupFinalizationRegistry ( _finalizationRegistry_ [ , _callback_ ] ) </h1>
    <p> The following steps are performed: </p>
    <emu-alg>
-      1. Assert: _finalizationRegistry_ has [[Cells]],
-      [[CleanupCallback]], and [[IsFinalizationRegistryCleanupJobActive]] internal slots.


I think you still need [[IsFinalizationRegistryCleanupJobActive]] to prevent re-entrancy through cleanupSome.

@mhofman I see how this is an observable difference and we could keep that, but what would be the problem if cleanupSome is called in a reentrant way?

It depends on what the original concerns with re-entrancy were.

If it was because the program could end up with 2 active iterators, then yes we don't need this check anymore. But in that case we could have allowed re-entrancy and just correctly tracked the currently "active" iterator instead of throwing on cleanupSome.

If it was to avoid having 2 internal iterations over the empty cells, then the re-entrancy check is still needed. It feels like it could be complicated for implementers to support right, I'll let them voice their opinion on this.

I don't think the latter was a concern, but it'd be good if implementers told me whether this assumption is valid. I think the concern was really just at the level of, how do the iterators even work if leaked--that you should only be able to iterate on them during the cleanup task, and not before or after. In this sense, [[IsFinalizationRegistryCleanupJobActive]] may have already been somewhat overkill.

I don't see an issue with recursively calling cleanupSome without iterators to worry about. It's an odd way to iterate through holdings but I don't see any problems really.

Co-Authored-By: Jordan Harband <[email protected]>

syg

lgtm with some wording suggestions

README.md

Co-Authored-By: Shu-yu Guo <[email protected]>

syg · 2020-03-09T22:32:18Z

spec/abstract-jobs.html

+      1. While _finalizationRegistry_.[[Cells]] contains a Record _cell_ such that _cell_.[[WeakRefTarget]] is ~empty~, then an implementation may perform the following steps,
+        1. Choose any such _cell_.
+        1. Remove _cell_ from _finalizationRegistry_.[[Cells]].
+        1. Perform ? Call(_callback_, *undefined*, &laquo; _cell_.[[HeldValue]] &raquo;).


Reading this again, this termination behavior isn't quite what I envisioned wrt backpressure on the cleanup tasks.

It's correct for cleanupSome to propagate the error up, but for the cleanup task, @mhofman's right in that letting exceptions interrupt is technically the same as letting the user express backpressure to the engine. The question to the engine is the same: if it has an interrupted iteration in the cleanup task, what should it do with that FR? Should the engine reschedule it? What if the user cleanup function always throws? Should the engine drop the FR after N tries?

I was envisioning something like in the background task, that all [[Cells]] in the FR will be iterated even if individual calls to cleanup throw. window.onerror will be called once each for the error.

Actually, I guess according to the current spec an implementation interrupts iteration is indistinguishable from an implementation that doesn't. The implementation that doesn't interrupt is equivalent to immediately running cleanup again after the current iteration is interrupted.

If this is intentional (which I'm happy with if so), then this deserves a note.

Ah wait, no, it's distinguishable in observing when the microtask checkpoint is performed...

I think cleanupSome will just trigger one iteration, so it's even more distinguishable. With the automatically scheduled thing, yeah, we don't give flexibility on the microtask checkpoint around reporting an error that we could decide to give.

One path here would be to collect all the errors and throw an AggregateError if there's more than one (regardless of whether it's from cleanupSome or the background scheduler). What do you think of those semantics?

After thinking about this more and discussing it with @syg offline, my (our?) current thought is:

AggregateError is unnecessary complexity; just throwing the first exception as the current PR does is a good solution.

It's OK to exit iteration early due to an exception, as long as we keep making forward progress. Each time, we'll have at least one iteration, so that's guaranteed (both with cleanupSome and with background calls).

Implementations could maintain the logic for background threads, to keep calling the cleanup callback with all the items they have, with the simple addition of a microtask checkpoint required after any exception thrown (which they are allowed to do anyway even if there's no exception). With this strategy, no requeueing is required in the background case.

littledan · 2020-03-11T10:03:50Z

Does anyone want to work on a test262 PR for this change? It might be easiest to develop in the context of an actual implementation; I'm not sure if anyone's drafted one yet.

syg · 2020-03-12T00:16:19Z

It might be easiest to develop in the context of an actual implementation; I'm not sure if anyone's drafted one yet.

I have a totally untested V8 implementation on the finalizationregistry-callback branch, for those interested. Don't trust it being correct please!

syg · 2020-03-13T20:56:10Z

Actually I'll do the test262 PR.

codehag · 2020-03-26T16:55:40Z

Hi Folks, would it be ok to push this to the last day for discussion?

syg · 2020-03-26T22:28:38Z

Hi Folks, would it be ok to push this to the last day for discussion?

I don't mind, but it is very important that we resolve it during the March meeting.

devsnek · 2020-04-02T00:51:29Z

this achieved consensus and the tests are merged 🎉

…ls. r=jonco Implements the spec changes from: tc39/proposal-weakrefs#187 The spec change removes the `FinalizationRegistryCleanupIterator` in favour of calling the clean-up callback for each finalised value. It also allows to call `cleanupSome()` within the callback function. `FinalizationRegistryObject::cleanupQueuedRecords()` has been changed to iterate from back to front, because this allows us to call `GCVector::popCopy()`, which makes it more efficient to remove entries from the `records` vector. Differential Revision: https://phabricator.services.mozilla.com/D70821

littledan added 3 commits February 21, 2020 15:52

Normative: Replace finalization iterator with multiple callback calls

81683b3

Rename to FinalizationRegistry in the README

070198c

Update explainer for one-off FinalizationRegistry calls

68474f2

littledan mentioned this pull request Feb 21, 2020

Finalization group iterator vs Array #60

Closed

ljharb reviewed Feb 21, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

mhofman reviewed Feb 22, 2020

View reviewed changes

mhofman suggested changes Feb 22, 2020

View reviewed changes

Daniel Ehrenberg and others added 3 commits February 22, 2020 11:08

Update README.md

342754f

Co-Authored-By: Jordan Harband <[email protected]>

Update README.md

ed32632

Co-Authored-By: Jordan Harband <[email protected]>

Update README.md

f1e7cfb

Co-Authored-By: Jordan Harband <[email protected]>

syg approved these changes Feb 24, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Daniel Ehrenberg and others added 2 commits February 25, 2020 12:14

Update README.md

996882a

Co-Authored-By: Shu-yu Guo <[email protected]>

Update README.md

f286686

Co-Authored-By: Shu-yu Guo <[email protected]>

littledan mentioned this pull request Mar 4, 2020

Soft check for WeakRef with reclaimed referent? #188

Closed

syg reviewed Mar 9, 2020

View reviewed changes

syg mentioned this pull request Mar 13, 2020

Migrate FinalizationRegistry tests to per-item callback API tc39/test262#2531

Merged

Merge branch 'master' into individual-callback

22be950

syg merged commit df16d7d into tc39:master Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Replace finalization iterator with multiple callback calls #187

Normative: Replace finalization iterator with multiple callback calls #187

littledan commented Feb 21, 2020

mhofman Feb 22, 2020

littledan Feb 22, 2020

mhofman Feb 23, 2020

littledan Feb 25, 2020

mhofman Feb 22, 2020

littledan Feb 22, 2020

mhofman Feb 23, 2020

littledan Feb 23, 2020

syg Feb 24, 2020

syg left a comment

syg Mar 9, 2020

syg Mar 9, 2020 •

edited

Loading

syg Mar 9, 2020

littledan Mar 10, 2020

littledan Mar 11, 2020 •

edited

Loading

littledan commented Mar 11, 2020

syg commented Mar 12, 2020

syg commented Mar 13, 2020

codehag commented Mar 26, 2020

syg commented Mar 26, 2020

devsnek commented Apr 2, 2020


		The `FinalizationGroup` callback is passed an iterator of held values to give that callback control over how much work it wants to process. The callback may pull in only part of the iterator, and in this case, the rest of the work would be "saved for later". The callback is not called during execution of other JavaScript code, but rather "in between turns"; it is currently proposed to be restricted to run after all of the `Promise`-related work is done, right before turning control over to the event loop.
		The `FinalizationRegistry` callback is called potentially multiple times, for each registered object that becomes dead, with a relevant held value. The callback is not called during execution of other JavaScript code, but rather "in between turns"--it is not interspersed with Promise work, for example, but only runs after all of the Promises have been processed.

Normative: Replace finalization iterator with multiple callback calls #187

Normative: Replace finalization iterator with multiple callback calls #187

Conversation

littledan commented Feb 21, 2020

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

syg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syg Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

littledan commented Mar 11, 2020

syg commented Mar 12, 2020

syg commented Mar 13, 2020

codehag commented Mar 26, 2020

syg commented Mar 26, 2020

devsnek commented Apr 2, 2020

syg Mar 9, 2020 •

edited

Loading

littledan Mar 11, 2020 •

edited

Loading