Skip to content
This repository was archived by the owner on Jan 25, 2022. It is now read-only.

Normative: not live FinalizationGroup included in Execution#147

Closed
mhofman wants to merge 1 commit intotc39:masterfrom
mhofman:finalizationgroup-liveness-execution
Closed

Normative: not live FinalizationGroup included in Execution#147
mhofman wants to merge 1 commit intotc39:masterfrom
mhofman:finalizationgroup-liveness-execution

Conversation

@mhofman
Copy link
Copy Markdown
Member

@mhofman mhofman commented Jun 29, 2019

Require implementations to treat FinalizationGroup objects the same during the Execution, whether they are live or not.
The goal is to make sure that the cleanup callback is invoked for an un-referenced FinalizationGroup if it would have been invoked had the FinalizationGroup been referenced.

Implementations can accomplish this by keeping FinalizationGroup objects alive while they have non-empty cells. After the discussion in #146, it became clear that the spec couldn't word it that way because performing the HostCleanupFinalizationGroup job is optional. This PR instead puts conditions on the optionality.

Fixes #66.

@mhofman
Copy link
Copy Markdown
Member Author

mhofman commented Jun 29, 2019

Let me know if the wording can be improved in any way!

Also, wondering if the section could use an editor's note saying that implementations can accomplish this by keeping FinalizationGroup objects alive while they have non-empty cells.

@littledan
Copy link
Copy Markdown
Member

littledan commented Oct 17, 2019

I'm not sure if a note is appropriate here: if you're saying "must", that should probably be outside of a note. But the question is, is this text implied by/redundant with the existing text? I'm not really sure why this PR is necessary. If it's not redundant, we should probably add normative text.

@mhofman
Copy link
Copy Markdown
Member Author

mhofman commented Oct 18, 2019

But the question is, is this text implied by/redundant with the existing text? If it's not redundant, we should probably add normative text.

The problem described in #66 exists in V8 today: an unreachable FinalizationGroup never invokes the callback. In that issue's comments, I and others believe that the liveness of the FinalizationGroup should have no impact on whether the callback is invoked or not.

I just added a test to my shim to highlight this: mhofman/tc39-weakrefs-shim@3cd6ff1

My understanding is that the spec as currently worded actually allows V8's current behavior.
The Execution step lets the implementer optionally perform HostCleanupFinalizationGroup(fg). That means the implementer is at liberty to perform the step for fg1 but not for fg2.

In my opinion, that optionality should be restricted to the implementation always performing HostCleanupFinalizationGroup, or never. I'm just not sure how to word that.

Or maybe I'm reading too much into "optionally", and the current text already implies this, in which case the note would be enough?

There is also a smaller escape hatch in the wording of the HostCleanupFinalizationGroup operation:

that is expected to call CleanupFinalizationGroup(fg) at some point in the future, if possible.

However I understand this as a SHOULD, and only in edge cases such as program termination, the host won't be able to call CleanupFinalizationGroup(fg). As such, that section is probably fine as is.

@littledan
Copy link
Copy Markdown
Member

If you're making a normative requirement, you need to put it outside of a note. Notes are just for context, redundant descriptions, examples, etc., not for making a requirement on implementations.

@mhofman
Copy link
Copy Markdown
Member Author

mhofman commented Oct 18, 2019

Oh yes, sorry I know this PR needs to be updated, I'm just not sure how.

Any suggestions?

@littledan
Copy link
Copy Markdown
Member

After thinking about this some more, I do like the idea of including this all in a note. The main rewording I would do is to replace "must" with statements of fact, along the lines of "the above algorithm implies that...".

This is all assuming that implementations are OK with these sorts of definitions, and not hoping to let the finalization group just die in these sorts of cases.

@littledan
Copy link
Copy Markdown
Member

Here's a PR which does that: #169

@mhofman
Copy link
Copy Markdown
Member Author

mhofman commented Nov 21, 2019

Here's a PR which does that: #169

I like the wording of #169 so closing this!

Edit: I still have concerns that Optionally allows the implementation to behave differently for different FinalizationGroup instances but that's a broader issue than just the live / not live FG instances described in #66 .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collection of FinalizationGroup objects

2 participants