ffi: keep DynamicLibrary alive while callbacks are registered

doc/api/ffi.md

@@ -482,6 +482,12 @@ When `signature` is omitted, the callback uses a default `void ()` signature.
 The return value is the callback pointer address as a `bigint`. It can be
 passed to native functions expecting a callback pointer.
 
+While a library has registered callbacks, it is not garbage collected, even if
+no JavaScript references to it remain. This keeps callback pointers held by
+native code valid until `library.unregisterCallback(pointer)` or
+`library.close()` is called. Unregister callbacks or close the library when
+they are no longer needed to avoid leaking it.
+
 ```cjs
 const { DynamicLibrary, suffix } = require('node:ffi');
 

src/node_ffi.cc

@@ -62,12 +62,24 @@ void FFIFunctionInfo::MemoryInfo(MemoryTracker* tracker) const {
 DynamicLibrary::DynamicLibrary(Environment* env, Local<Object> object)
     : BaseObject(env, object) {
   MakeWeak();
+  env->AddCleanupHook(CleanupHook, this);
 }
 
 DynamicLibrary::~DynamicLibrary() {
+  env()->RemoveCleanupHook(CleanupHook, this);
   this->Close();
 }
 
+void DynamicLibrary::CleanupHook(void* arg) {
+  DynamicLibrary* lib = static_cast<DynamicLibrary*>(arg);
+  // Registered callbacks hold strong references to the library, which would
+  // otherwise keep it alive past Environment teardown. Release them only
+  // after Close() returns, since dropping the last reference may delete lib.
+  decltype(lib->callbacks_) callbacks;
+  callbacks.swap(lib->callbacks_);
+  lib->Close();
+}
+
 bool DynamicLibrary::is_closed() const {
   return static_cast<void*>(lib_.handle) == nullptr;
 }
@@ -1100,7 +1112,7 @@ void DynamicLibrary::RegisterCallback(const FunctionCallbackInfo<Value>& args) {
   }
 
   auto callback = std::unique_ptr<FFICallback>(
-      new FFICallback{.owner = lib,
+      new FFICallback{.owner = BaseObjectPtr<DynamicLibrary>(lib),
                       .env = env,
                       .thread_id = std::this_thread::get_id(),
                       .fn = Global<Function>(isolate, fn),

src/node_ffi.h

@@ -83,7 +83,10 @@ class FFIFunctionInfo final : public BaseObject {
 };
 
 struct FFICallback {
-  DynamicLibrary* owner;
+  // Callback pointers are plain addresses that cannot root the library, so
+  // each registered callback keeps its owner alive until it is unregistered
+  // or the library is closed.
+  BaseObjectPtr<DynamicLibrary> owner;
   Environment* env;
   std::thread::id thread_id;
   v8::Global<v8::Function> fn;
@@ -161,6 +164,7 @@ class DynamicLibrary : public BaseObject {
       Environment* env,
       const std::string& name,
       const std::shared_ptr<FFIFunction>& fn);
+  static void CleanupHook(void* arg);
   static void CleanupFunctionInfo(
       const v8::WeakCallbackInfo<FFIFunctionInfo>& data);
   bool is_closed() const;

test/ffi/test-ffi-callback-gc.js

@@ -0,0 +1,90 @@
+// Flags: --expose-gc
+'use strict';
+const { skipIfFFIMissing } = require('../common');
+skipIfFFIMissing();
+
+const { gcUntil } = require('../common/gc');
+const test = require('node:test');
+const ffi = require('node:ffi');
+const { fixtureSymbols, libraryPath } = require('./ffi-test-common');
+
+const signature = { arguments: ['i32'], return: 'i32' };
+
+test('registered callbacks keep an unreachable library alive', async (t) => {
+  // A separate handle provides the native function that invokes the callback.
+  const { lib: keeper, functions: symbols } =
+    ffi.dlopen(libraryPath, fixtureSymbols);
+  t.after(() => keeper.close());
+
+  let lib = new ffi.DynamicLibrary(libraryPath);
+  const ref = new WeakRef(lib);
+  const pointer = lib.registerCallback(signature, (x) => x * 2);
+  t.assert.strictEqual(symbols.call_int_callback(pointer, 21), 42);
+
+  lib = null;
+  for (let i = 0; i < 5; i++) {
+    await gcUntil('library with callbacks stays alive', () => true, 1);
+    t.assert.strictEqual(typeof ref.deref(), 'object');
+    t.assert.strictEqual(symbols.call_int_callback(pointer, 21), 42);
+  }
+
+  ref.deref().unregisterCallback(pointer);
+});
+
+test('library becomes collectable after its callbacks are unregistered', async () => {
+  let lib = new ffi.DynamicLibrary(libraryPath);
+  const ref = new WeakRef(lib);
+  const first = lib.registerCallback(signature, (x) => x);
+  const second = lib.registerCallback(signature, (x) => x);
+  lib.unregisterCallback(first);
+  lib.unregisterCallback(second);
+  lib = null;
+
+  await gcUntil('library is collected after unregisterCallback',
+                () => ref.deref() === undefined);
+});
+
+test('library becomes collectable after close()', async () => {
+  let lib = new ffi.DynamicLibrary(libraryPath);
+  const ref = new WeakRef(lib);
+  lib.registerCallback(signature, (x) => x);
+  lib.close();
+  lib = null;
+
+  await gcUntil('library is collected after close()',
+                () => ref.deref() === undefined);
+});
+
+test('unrefCallback() does not release the callback trampoline', async (t) => {
+  const { lib: keeper, functions: symbols } =
+    ffi.dlopen(libraryPath, fixtureSymbols);
+  t.after(() => keeper.close());
+
+  let lib = new ffi.DynamicLibrary(libraryPath);
+  const ref = new WeakRef(lib);
+  const pointer = lib.registerCallback(signature, () => 1);
+  lib.unrefCallback(pointer);
+  lib = null;
+
+  for (let i = 0; i < 5; i++) {
+    await gcUntil('library with unref callbacks stays alive', () => true, 1);
+    t.assert.strictEqual(typeof ref.deref(), 'object');
+  }
+  // The JS function may be gone, but the trampoline must still be callable.
+  symbols.call_int_callback(pointer, 21);
+
+  ref.deref().unregisterCallback(pointer);
+});
+
+test('libraries with registered callbacks do not block teardown', async (t) => {
+  const { Worker } = require('node:worker_threads');
+  const { once } = require('node:events');
+  const worker = new Worker(`
+    const ffi = require('node:ffi');
+    const lib = new ffi.DynamicLibrary(${JSON.stringify(libraryPath)});
+    lib.registerCallback(${JSON.stringify(signature)}, (x) => x);
+    globalThis.lib = lib;
+  `, { eval: true });
+  const [code] = await once(worker, 'exit');
+  t.assert.strictEqual(code, 0);
+});