NativeFinalizer and Finalizer: Should calling detach throw an error instead of be a no-op if nothing is detached?

[dcharkes]

Trying to detach from a non-shared static nativeFinalizer would lead to a no-op

Wouldn't it throw a nice exception: "You cannot detach object foo from this finalizer as it was not attached" ?

Originally posted by @mkustermann in #56227 (comment)

Changing it now, would be a breaking change.

Also, it would mean that code racing with the GC to detach/run the finalizer if you only hold on to the detach key would start throwing if the finalizer is run first. Arguably, such code is likely broken.

I don't remember if we discussed this, and if we made it a no-op for a specific reason. (I don't see any discussion on https://dart-review.googlesource.com/c/sdk/+/238086.) Maybe @lrhn remembers.

cc @rakudrama @sigmundch for Finalizer on JS backend.

[dart-github-bot]

Summary: The issue proposes that calling detach on a NativeFinalizer or Finalizer when nothing is attached should throw an error instead of being a no-op. This change would provide more informative feedback to developers, but it could also introduce breaking changes and potential race conditions.

[mkustermann]

Also, it would mean that code racing with the GC to detach/run the finalizer if you only hold on to the detach key would start throwing if the finalizer is run first. Arguably, such code is likely broken.

Ah, the finalizer detach API doesn't require the object that has a finalizer attached, so that makes this probably not possible.

[lrhn]

Agree. The detach-token object is not the trigger-object, so the entry can go away while you're still holding the detach token. You can't know ahead of time whether there is an entry with that detach token or not (at least not unless you do something to count in the callback).

We don't want the finalizer to retain any state after the trigger object has been GC'ed and the callback invoked.
(Also, it's how the JS finalizer works, so we don't want to work differently. And they do it for all the same reasons.)

And if we wanted to fail if removing something that wasn't there, it would just mean we had to have a way to check,
and the safe detach code would become if (finalizer.hasDetach(key)) finalizer.detach(key);.
That doesn't bring me joy.