From 6f927393ea957bde2cabe5f8e2a92601655a2c73 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Thu, 22 Jun 2023 15:19:57 -0400 Subject: [PATCH 1/4] Suggest also using a FinalizationRegistry when a Disposable holds a native handle --- spec.emu | 1 + 1 file changed, 1 insertion(+) diff --git a/spec.emu b/spec.emu index a407eea..5235d8d 100644 --- a/spec.emu +++ b/spec.emu @@ -4313,6 +4313,7 @@ contributors: Ron Buckton, Ecma International

Invoking this method notifies the Disposable 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.

If called more than once on the same object, the function should not throw an exception. However, this requirement is not enforced.

When using a Disposable 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.

+

When implementing a Disposable 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 Disposable 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.

From a1b3f0e0b94574c80a4388e21db6b41e3a9724f4 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Thu, 22 Jun 2023 16:05:13 -0400 Subject: [PATCH 2/4] Add SafeFileHandle example to the README --- README.md | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/README.md b/README.md index 1ed96e4..36ca28b 100644 --- a/README.md +++ b/README.md @@ -1799,6 +1799,125 @@ 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 +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); + + #unregisterToken = {}; + #handle; + + constructor(file) { + this.#handle = nativeOpenFile(file); + + // Register for finalization + SafeHandle.#finalizer.register(this, handle, this.#unregisterToken); + } + + 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.#unregisterToken); + + // 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) From 521f766cae52e4f46792416a7dde60fb5177a1ce Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Thu, 22 Jun 2023 17:20:59 -0400 Subject: [PATCH 3/4] Use 'this' as unregisterToken in example --- README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 36ca28b..2d4416d 100644 --- a/README.md +++ b/README.md @@ -1823,14 +1823,13 @@ function nativeCloseFile(handle) { ... } class SafeFileHandle { static #finalizer = new FinalizationRegistry(nativeCloseFile); - #unregisterToken = {}; #handle; constructor(file) { this.#handle = nativeOpenFile(file); // Register for finalization - SafeHandle.#finalizer.register(this, handle, this.#unregisterToken); + SafeHandle.#finalizer.register(this, handle, /*unregisterToken*/ this); } get disposed() { @@ -1845,7 +1844,7 @@ class SafeFileHandle { dispose() { if (this.#handle !== 0) { // This instance no longer needs finalization - SafeHandle.#finalizer.unregister(this.#unregisterToken); + SafeHandle.#finalizer.unregister(this); // Perform cleanup const handle = this.#handle; From b0ecb695951f3a2ae3988f2b7d21facc960850b3 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Fri, 23 Jun 2023 13:06:43 -0400 Subject: [PATCH 4/4] Fix typos in README.md --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2d4416d..fea6564 100644 --- a/README.md +++ b/README.md @@ -1829,7 +1829,7 @@ class SafeFileHandle { this.#handle = nativeOpenFile(file); // Register for finalization - SafeHandle.#finalizer.register(this, handle, /*unregisterToken*/ this); + SafeFileHandle.#finalizer.register(this, this.#handle, /*unregisterToken*/ this); } get disposed() { @@ -1844,7 +1844,7 @@ class SafeFileHandle { dispose() { if (this.#handle !== 0) { // This instance no longer needs finalization - SafeHandle.#finalizer.unregister(this); + SafeFileHandle.#finalizer.unregister(this); // Perform cleanup const handle = this.#handle;