Suggest also using a FinalizationRegistry when a Disposable holds a native handle

[rbuckton]

This adds a non-normative note to suggest that it is good practice to also use a FinalizationRegistry when using @@dispose to implement a disposable object that holds a native handle, so as to ensure the handle is properly closed if the disposable object is garbage collected without having been disposed.

Including this suggestion is based on this discussion: #159 (comment)

cc: @tc39/ecma262-editors

[github-actions]

A preview of this PR can be found at https://tc39.es/proposal-explicit-resource-management/pr/168.

[bakkot]

I don't think I'd want to suggest this as good practice in general - FinalizationRegistry is pretty heavy-weight, and also if you do this users may come to rely on it instead of doing their own cleanup, which would be bad. Maybe suggest that you can have a debug-only version which uses FinalizationRegistry to help locate leaks?

To put it another way: this advice is basically orthogonal to the proposal, since string-named close methods which the user must call are already widespread, including for things backed by native handles. And I would not recommend defaulting to the use of a FinalizationRegistry when producing such resources today.

[rbuckton]

It is pretty heavy weight, but is only really suggested for use with native handles. I imagine most hosts will do this for native handle wrappers, though possibly not by using FinalizationRegistry when they can leverage host APIs for GC. It's generally not necessary when composing other disposables, since only the disposables that wrap unmanaged resources need to care about avoiding leaks.

To put it another way: this advice is basically orthogonal to the proposal, since string-named close methods which the user must call are already widespread, including for things backed by native handles.

I imagine most host objects that provide a close() today already have mechanisms for avoiding leaks in a similar fashion. For example, NodeJS's FileHandle automatically closes the handle when GC'd, per https://github.com/nodejs/node/blob/1f4b0c056cfa19a40b3b69d9fb5af3f1c2c1af2e/src/node_file.h#L302-L304

[rbuckton]

For additional reference, here is the guidance on implementing a Dispose() method in C#: https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose.

I've also considered simplifying this by proposing something like a SafeHandle built-in as a follow-on proposal, not unlike https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle, since this approach can be generalized for any handle value that can't be weakly held.

[rbuckton]

A rough example of various implementations of a general-purpose SafeHandle can be found here: https://gist.github.com/rbuckton/49e2d4a5e2b691d393f146f8c1b8f19a

[mhofman]

I've also considered simplifying this by proposing something like a SafeHandle built-in as a follow-on proposal

I'd prefer to not have new ways to observe GC, but we can debate that when that proposal is made.

[rbuckton]

I've also considered simplifying this by proposing something like a SafeHandle built-in as a follow-on proposal

I'd prefer to not have new ways to observe GC, but we can debate that when that proposal is made.

I wouldn't classify it as a new way to observe GC, per se, since it's essentially the same way given that any userland library can implement the same thing on top of FinalizationRegistry.

[mhofman]

Let me rephrase, I'd prefer to avoid a new built-in API which is GC sensitive. If that API behaves the same as and can be entirely polyfilled using existing APIs, then in my opinion it belongs to userland. I do hope to someday introduce a new GC sensitive API to the language, but the purpose would be to enable distributed GC, and that cannot be polyfilled.

[rixtox]

Why only a good practice for native handles? If I were to implement a Disposable to let say removeEventListener() when disposed, and use it everywhere to replace addEventListener() calls, I would definitely want to make sure it can be disposed on GC in case someone forget to use it, and print out a warning if that happens.

[rbuckton]

While that's a valid case, that leans more towards @bakkot's suggestion that such things be reserved for debug builds. Native handles are often a different story as such leaks can potentially cause system instability. Also, callbacks registered via addEventListener would be visible in a heap dump/snapshot which makes such leaks easier to find than a stray Number value.

[yw662]

You need to make it a weakRef in that scenario, however EventTarget do not hold weakRefs.

[rbuckton]

This was discussed during the July, 2023 plenary and the general sentiment expressed at that time matched @bakkot's comment above. No recommendation will be made within the specification at this time.

I may still amend the explainer with some suggestions on how to handle "unused" resources, however. FinalizationRegistry could still be used to either perform implicit cleanup for linear chunks of memory in WASM, or to report a warning to the console about failed cleanup.

[guybedford]

Thanks for the pointers. The GC integration still seems a little unexplored to me as a leak solution. Could engines go a step further here and call Symbol.dispose as part of normal GC cleanup, separately to treating it as a finalization registry feature? Or at least sharing some of the spec pieces, without explicitly requiring the same registration mechanisms.

I honestly can't think of a scenario in which this would not be the desired behaviour. Pretty much all low-level object wraps in Node.js act this way too.

[mhofman]

Could engines go a step further here and call Symbol.dispose as part of normal GC cleanup

That's not possible as it would be equivalent to a destructor (not a finalizer), which would still have access to object, and could introduce revival issues. Also running user code during GC is a no go.

The behavior of finalizers is to optimistically run sometime after the object has been collected. With some kind of automatic disposal on finalization, you'd basically need every object ever created to automatically be probed for a finalizer, which would capture some internal data but somehow not the instance itself, and get executed after the object is collected.

[guybedford]

Yeah that makes sense. The problem though is that Symbol.dispose needs to unregister the finalizer in the finalization registry, and so has to track the finalization token, so you end up with Symbol.dispose as an instance method any way to support these use cases.

I just implemented this in https://github.com/bytecodealliance/jco/pull/357/files#diff-bbd0007a3c51b74cbe102f3e0a555d9e3b61ed661726b6e79b7b83a6152c57b3R114.

[guybedford]

I suppose this just is the common pattern - where the finalization token has to be stored on the instance for deregistration in Symbol.dispose. The important point from a spec perspective is that the primitives are there to do it.

I've perhaps come around to this not being so bad after all!

[mhofman]

You can use the instance itself as the registration token. No need for a separate token.

[guybedford]

The issue is you need to track the deregistration token, so that calling Symbol.dispose early will deregister the finalizer and have access to this token from the instance. But I think it works out quite naturally after all.

[rbuckton]

This exact example can be found here:

#159 (comment)

and here:

#159 (comment)