fix(bindgen): resource disposal

crates/js-component-bindgen/src/function_bindgen.rs

@@ -2300,25 +2300,26 @@ impl Bindgen for FunctionBindgen<'_> {
                                 uwriteln!(self.src,
                                             "Object.defineProperty({rsc}, {symbol_resource_handle}, {{ writable: true, value: {handle} }});
                                     finalizationRegistry{tid}.register({rsc}, {handle}, {rsc});");
-                                if let Some(dtor) = dtor_name {
-                                    // The Symbol.dispose function gets disabled on drop, so we can rely on the own handle remaining valid.
-                                    uwriteln!(
-                                                self.src,
-                                                "Object.defineProperty({rsc}, {symbol_dispose}, {{ writable: true, value: function () {{
-                                            finalizationRegistry{tid}.unregister({rsc});
-                                            {rsc_table_remove}(handleTable{tid}, {handle});
-                                            {rsc}[{symbol_dispose}] = {empty_func};
-                                            {rsc}[{symbol_resource_handle}] = undefined;
-                                            {dtor}(handleTable{tid}[({handle} << 1) + 1] & ~{rsc_flag});
-                                        }}}});"
-                                            );
-                                } else {
-                                    // Set up Symbol.dispose for borrows to allow its call, even though it does nothing.
-                                    uwriteln!(
-                                        self.src,
-                                        "Object.defineProperty({rsc}, {symbol_dispose}, {{ writable: true, value: {empty_func} }});",
-                                    );
-                                }
+                                let dtor_call = dtor_name
+                                    .as_ref()
+                                    .map(|dtor| format!("{dtor}(handleEntry.rep);"))
+                                    .unwrap_or_default();
+                                // Explicitly dropping an own handle must always release the host-side
+                                // handle and finalizer registration. A component-defined destructor is
+                                // an additional callback, not a prerequisite for resource cleanup.
+                                //
+                                // Disable Symbol.dispose and clear the handle before calling the
+                                // component destructor so repeated or re-entrant disposal is a no-op.
+                                uwriteln!(
+                                            self.src,
+                                            "Object.defineProperty({rsc}, {symbol_dispose}, {{ writable: true, value: function () {{
+                                        finalizationRegistry{tid}.unregister({rsc});
+                                        const handleEntry = {rsc_table_remove}(handleTable{tid}, {handle});
+                                        {rsc}[{symbol_dispose}] = {empty_func};
+                                        {rsc}[{symbol_resource_handle}] = undefined;
+                                        {dtor_call}
+                                    }}}});"
+                                        );
                             } else {
                                 // Borrow handles of local resources have rep handles, which we carry through here.
                                 uwriteln!(

crates/js-component-bindgen/src/ts_bindgen.rs

@@ -876,9 +876,11 @@ impl<'a> TsInterface<'a> {
 
     fn finish(mut self) -> (Source, BTreeSet<String>) {
         for (resource, (meta, source)) in self.resources {
-            let (impl_phrase, extra_members) = if self.is_guest && meta.is_import() {
-                // Resources types imported in the guest  will have generated [Symbol.dispose]()
-                // for resource cleanup, but this is not the case for any other scenario.
+            let generates_disposable_resource =
+                (self.is_guest && meta.is_import()) || (!self.is_guest && meta.is_export());
+            let (impl_phrase, extra_members) = if generates_disposable_resource {
+                // Guest imports and host exports are resource consumers. Their generated
+                // wrappers own component-model handles and support deterministic cleanup.
                 (" implements Disposable", vec!["[Symbol.dispose](): void;"])
             } else {
                 ("", vec![])

packages/jco-transpile/test/fixtures/wit/disposable-resources/disposable-resources.wit

@@ -18,8 +18,17 @@ interface resources {
   create-connection: func(url: string) -> connection;
 }
 
+interface exported-resources {
+  /// A component-defined resource consumed by the host.
+  resource transaction {
+    constructor();
+    commit: func();
+  }
+}
+
 world disposable-world {
   import resources;
+  export exported-resources;
 
   /// Also test a world-level resource
   resource database {
@@ -28,4 +37,4 @@ world disposable-world {
   }
 
   export run: func() -> string;
-}
+}

packages/jco-transpile/test/p3/future-lifts.ts

@@ -436,6 +436,17 @@ suite('future<T> lifts', () => {
                 instance['jco:test-components/get-future-async'].getExampleResourceOwnDisposes(),
                 numDisposed,
             );
+            assert.doesNotThrow(() => resource[disposeSymbol](), 'resource disposal should be idempotent');
+            assert.strictEqual(
+                instance['jco:test-components/get-future-async'].getExampleResourceOwnDisposes(),
+                numDisposed,
+                'disposing the same handle twice must not run the guest destructor twice',
+            );
+            assert.throws(
+                () => resource.getId(),
+                /not a valid .* resource/i,
+                'a disposed resource wrapper must no longer be usable',
+            );
         }
     });
 

packages/jco-transpile/test/resources.ts

@@ -1,16 +1,23 @@
+import { readFile } from 'node:fs/promises';
 import { join } from 'node:path';
+import { fileURLToPath } from 'node:url';
 
-import { suite, test } from 'vitest';
+import { assert, suite, test } from 'vitest';
 
 import { WASIShim } from '@bytecodealliance/preview2-shim/instantiation';
 
+import { transpileBytes } from '../src/index.js';
+import { componentEmbed, componentNew } from '../src/wasm-tools.js';
 import { setupAsyncTest } from './helpers.js';
 import { LOCAL_TEST_COMPONENTS_DIR } from './common.js';
 
 suite('resources', () => {
     // Ensure trampoline code is called on external object with relevant `this`.
     // see: https://github.com/bytecodealliance/jco/issues/1313
     test('simple imported resource call', async () => {
+        const disposeSymbol = Symbol.dispose || Symbol.for('dispose');
+        let disposed = 0;
+
         class IncrementingExampleResource {
             private id: number;
 
@@ -22,6 +29,10 @@ suite('resources', () => {
                 this.id += 1;
                 return this.id;
             }
+
+            [disposeSymbol]() {
+                disposed += 1;
+            }
         }
 
         const name = 'resource-incrementing-id';
@@ -43,7 +54,62 @@ suite('resources', () => {
         });
 
         instance['jco:test-components/local-run'].run();
+        assert.strictEqual(disposed, 1, 'dropping the guest-owned import should dispose the host resource once');
 
         await cleanup();
     });
+
+    test('component-defined resource disposal uses the removed handle representation', async () => {
+        const fixture = fileURLToPath(
+            new URL('./fixtures/components/runtime/resources.2.component.wat', import.meta.url),
+        );
+        const { files } = await transpileBytes(await readFile(fixture), {
+            name: 'resource-disposal',
+            minify: false,
+        });
+        const source = new TextDecoder().decode(files['resource-disposal.js']);
+
+        assert.match(
+            source,
+            /const handleEntry = rscTableRemove\(handleTable\d+, handle\d+\);[\s\S]*?\['0'\]\(handleEntry\.rep\);/,
+            'explicit disposal should call the destructor with the representation returned while removing the handle',
+        );
+        assert.notMatch(
+            source,
+            /rscTableRemove\(handleTable\d+, handle\d+\);[\s\S]{0,300}?handleTable\d+\[\(handle\d+ << 1\) \+ 1\]/,
+            'explicit disposal must not read a representation from a removed handle-table entry',
+        );
+    });
+
+    test('component-defined resource without a destructor still releases its handle', async () => {
+        const component = await componentNew(
+            await componentEmbed({
+                dummy: true,
+                witSource: `
+                    package test:resource-disposal;
+
+                    interface resources {
+                        resource no-destructor {
+                            constructor();
+                        }
+                    }
+
+                    world test {
+                        export resources;
+                    }
+                `,
+            }),
+        );
+        const { files } = await transpileBytes(component, {
+            name: 'resource-without-destructor',
+            minify: false,
+        });
+        const source = new TextDecoder().decode(files['resource-without-destructor.js']);
+
+        assert.match(
+            source,
+            /Object\.defineProperty\(rsc\d+, symbolDispose,[\s\S]*?const handleEntry = rscTableRemove\(handleTable\d+, handle\d+\);[\s\S]*?symbolRscHandle\] = undefined;/,
+            'explicit disposal should release and invalidate an owned handle even without a guest destructor',
+        );
+    });
 });

packages/jco-transpile/test/typescript.ts

@@ -47,11 +47,7 @@ suite(`TypeScript`, async () => {
         assert.ok(dtsSource.includes(`export function foobarbaz(): Array<Value | undefined>;`));
     });
 
-    // NOTE: somewhat confusingly, host generation of resources should *not*
-    // generation Disposable, because the embedder may or may not choose to make
-    // the implemented resource import disposable or not.
-    //
-    test.concurrent(`Disposable interface generation (host-types, import) `, async () => {
+    test.concurrent(`Disposable interface generation (host types)`, async () => {
         const witSource = await readFile(
             fileURLToPath(new URL(`./fixtures/wit/disposable-resources/disposable-resources.wit`, import.meta.url)),
             'utf8',
@@ -70,7 +66,7 @@ suite(`TypeScript`, async () => {
             [`export class Database implements Disposable {`, `[Symbol.dispose](): void;`].every(
                 (s) => !mainDtsSource.includes(s),
             ),
-            'Database resource should not implement Disposable interface',
+            'host-provided world-level Database resource should not require Disposable',
         );
 
         const interfaceDtsSource = new TextDecoder().decode(
@@ -82,7 +78,17 @@ suite(`TypeScript`, async () => {
                 `export class Connection implements Disposable {`,
                 `[Symbol.dispose](): void;`,
             ].every((s) => !interfaceDtsSource.includes(s)),
-            'FileHandle/Connection resources should not implement Disposable interface',
+            'host-provided FileHandle/Connection resource imports should not require Disposable',
+        );
+
+        const exportedInterfaceDtsSource = new TextDecoder().decode(
+            files['interfaces/test-disposable-resources-exported-resources.d.ts'],
+        );
+        assert(
+            [`export class Transaction implements Disposable {`, `[Symbol.dispose](): void;`].every((s) =>
+                exportedInterfaceDtsSource.includes(s),
+            ),
+            'component-exported Transaction resource should implement Disposable',
         );
     });
 
@@ -113,6 +119,16 @@ suite(`TypeScript`, async () => {
             ].every((s) => interfaceDtsSource.includes(s)),
             'FileHandle/Connection resources should implement Disposable interface',
         );
+
+        const exportedInterfaceDtsSource = new TextDecoder().decode(
+            files['interfaces/test-disposable-resources-exported-resources.d.ts'],
+        );
+        assert(
+            [`export class Transaction implements Disposable {`, `[Symbol.dispose](): void;`].every(
+                (s) => !exportedInterfaceDtsSource.includes(s),
+            ),
+            'guest-provided Transaction implementation should not require Disposable',
+        );
     });
 
     // Regression test for https://github.com/bytecodealliance/jco/issues/1708

packages/jco/test/componentize.js

@@ -15,6 +15,38 @@ import { exec, getTmpDir, jcoPath } from "./helpers.js";
 // used widely, we can switch to regular dynamic imports, as componentize-js
 // versions are real dependencies now.
 suite("componentize", () => {
+    // ComponentizeJS currently omits the component-model destructor metadata for
+    // a JS-exported resource.
+    //
+    // TODO: flip this test once componentize-js is fixed
+    // see: https://github.com/bytecodealliance/jco/issues/989#issuecomment-4550236825
+    test.fails("componentized exported resource invokes its guest destructor", async () => {
+        const fixtureDir = join(COMPONENT_JS_FIXTURES_DIR, "resource-disposal");
+        const outputDir = await getTmpDir();
+        const componentPath = join(outputDir, "component.wasm");
+        const transpiledDir = join(outputDir, "transpiled");
+
+        await exec(
+            jcoPath,
+            "componentize",
+            join(fixtureDir, "source.js"),
+            "--disable",
+            "all",
+            "-w",
+            join(fixtureDir, "source.wit"),
+            "-o",
+            componentPath,
+        );
+        await exec(jcoPath, "transpile", componentPath, "-o", transpiledDir, "--name", "resource-disposal");
+        await writeFile(join(transpiledDir, "package.json"), JSON.stringify({ type: "module" }));
+
+        const { resources } = await import(`${pathToFileURL(transpiledDir)}/resource-disposal.js`);
+        const resource = new resources.Example(42);
+        assert.strictEqual(resource.getId(), 42);
+        resource[Symbol.dispose || Symbol.for("dispose")]();
+        assert.strictEqual(resources.disposeCount(), 1);
+    });
+
     test("rejects a custom StarlingMonkey engine with the QuickJS backend", async () => {
         const fixtureDir = join(COMPONENT_JS_FIXTURES_DIR, "typescript-direct");
         const outputDir = await getTmpDir();

packages/jco/test/fixtures/componentize/resource-disposal/source.js

@@ -0,0 +1,22 @@
+let disposeCount = 0;
+
+class Example {
+    constructor(id) {
+        this.id = id;
+    }
+
+    getId() {
+        return this.id;
+    }
+
+    [Symbol.dispose]() {
+        disposeCount += 1;
+    }
+}
+
+export const resources = {
+    Example,
+    disposeCount() {
+        return disposeCount;
+    },
+};

packages/jco/test/fixtures/componentize/resource-disposal/source.wit

@@ -0,0 +1,14 @@
+package local:resource-disposal;
+
+interface resources {
+  resource example {
+    constructor(id: u32);
+    get-id: func() -> u32;
+  }
+
+  dispose-count: func() -> u32;
+}
+
+world test {
+  export resources;
+}