tc39 / proposal-explicit-resource-management

ECMAScript Explicit Resource Management
https://arai-a.github.io/ecma262-compare/?pr=3000
BSD 3-Clause "New" or "Revised" License
725 stars 29 forks source link

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

Closed rbuckton closed 12 months ago

rbuckton commented 1 year ago

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: https://github.com/tc39/proposal-explicit-resource-management/issues/159#issuecomment-1588158664

cc: @tc39/ecma262-editors

github-actions[bot] commented 1 year ago

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

bakkot commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mhofman commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

rbuckton commented 12 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

guybedford commented 5 months ago

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 commented 5 months ago

This exact example can be found here:

https://github.com/tc39/proposal-explicit-resource-management/issues/159#issuecomment-1582838713

and here:

https://github.com/tc39/proposal-explicit-resource-management/issues/159#issuecomment-1584900038