Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
118 changes: 118 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1799,6 +1799,124 @@ suggestions for consideration. The actual implementation is at the discretion of
- `stream.Readable` — Either `@@dispose()` or `@@asyncDispose()` as an alias or [wrapper][] for `destroy()`.
- ... and many others in `net`, `readline`, `tls`, `udp`, and `worker_threads`.

# Best Practices

## Ensuring Cleanup of Native Handles, File Descriptors, etc.

When implementing a disposable that holds a native handle (i.e., a pointer, file descriptor, etc.), it is good practice
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

to also use a `FinalizationRegistry` to ensure the handle is appropriately closed if the disposable object is garbage
collected without having been disposed.

The following is an example of a safe handle wrapper that could be used in this case:

```js
/**
* @returns {number}
*/
function nativeOpenFile(file) { ... }

/**
* @param {number} handle
*/
function nativeCloseFile(handle) { ... }

class SafeFileHandle {
static #finalizer = new FinalizationRegistry(nativeCloseFile);

#handle;

constructor(file) {
this.#handle = nativeOpenFile(file);

// Register for finalization
SafeHandle.#finalizer.register(this, handle, /*unregisterToken*/ this);
rbuckton marked this conversation as resolved.
Show resolved Hide resolved
rbuckton marked this conversation as resolved.
Show resolved Hide resolved
}

get disposed() {
return this.#handle === 0;
}

dangerousGetHandle() {
if (this.#handle === 0) throw new ReferenceError("Object is disposed");
return this.#handle;
}

dispose() {
if (this.#handle !== 0) {
// This instance no longer needs finalization
SafeHandle.#finalizer.unregister(this);
rbuckton marked this conversation as resolved.
Show resolved Hide resolved

// Perform cleanup
const handle = this.#handle;
this.#handle = 0;
nativeCloseFile(handle);
}
}

[Symbol.dispose]() {
this.dispose();
}
}

/**
* @returns {SafeFileHandle}
*/
export function openFile(file) { ... }

/**
* @param {SafeFileHandle} handle
*/
export function readFile(handle) {
const fd = handle.dangerousGetHandle();
...
}
```

This ensures that `SafeFileHandle` will still implicitly close the underlying file if an instance is not disposed before
being garbage collected, while still allowing the user to explicitly close the file via `using` or an imperative call to
`safeFileHandle.dispose()`. The call to `finalizationRegistry.unregister` prevents `nativeCloseHandle` from being called
against an handle that may have already been closed or even reused by a different operation. The `dangerousGetHandle()`
method allows access to the underlying handle itself for use with other APIs that may still expect a `number`, but that
can be further protected through the use of `static {}` blocks to limit unsafe access to the handle:

```js
...

/**
* @type {(obj: SafeFileHandle) => number}
*/
let dangerousGetHandle; // unreachable outside of module

class SafeFileHandle {
#handle;

...

// dangerousGetHandle() {
// if (this.#handle === 0) throw new ReferenceError("Object is disposed");
// return this.#handle;
// }

static {
dangerousGetHandle = obj => {
if (obj.#handle === 0) throw new ReferenceError("Object is disposed");
return obj.#handle;
};
}

...
}

...

export function readFile(handle) {
// const fd = handle.dangerousGetHandle();
const fd = dangerousGetHandle(handle);
...
}

```

# Meeting Notes

* [TC39 July 24th, 2018](https://github.com/tc39/notes/blob/main/meetings/2018-07/july-24.md#explicit-resource-management)
Expand Down
1 change: 1 addition & 0 deletions spec.emu
Original file line number Diff line number Diff line change
Expand Up @@ -4313,6 +4313,7 @@ contributors: Ron Buckton, Ecma International
<p>Invoking this method notifies the <em>Disposable</em> object that the caller does not intend to continue to use this object. This method should perform any necessary logic to perform explicit clean-up of the resource including, but not limited to, file system handles, streams, host objects, etc. When an exception is thrown from this method, it typically means that the resource could not be explicitly freed.</p>
<p>If called more than once on the same object, the function should not throw an exception. However, this requirement is not enforced.</p>
<p>When using a <em>Disposable</em> object, it is good practice to create the instance with a `using` declaration, as the resource will be automatically disposed when the |Block| or |Module| immediately containing the declaration has been evaluated.</p>
<p>When implementing a <em>Disposable</em> object that holds a native handle (i.e., a pointer, a file descriptor, etc.), it is good practice to also use a `FinalizationRegistry` to ensure the handle is closed if the <em>Disposable</em> object is garbage collected without having been disposed, as well as use the `FinalizationRegistry`'s `unregister` method to prevent finalization if the object is disposed before it is garbage collected.</p>
</td>
</tr>
</tbody>
Expand Down