diff --git a/doc/api/ffi.md b/doc/api/ffi.md index d3c01f6e052..c0d27dd899d 100644 --- a/doc/api/ffi.md +++ b/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'); diff --git a/src/node_ffi.cc b/src/node_ffi.cc index 7581da4e7c6..be81342cb8e 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -62,12 +62,24 @@ void FFIFunctionInfo::MemoryInfo(MemoryTracker* tracker) const { DynamicLibrary::DynamicLibrary(Environment* env, Local 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(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(lib_.handle) == nullptr; } @@ -1100,7 +1112,7 @@ void DynamicLibrary::RegisterCallback(const FunctionCallbackInfo& args) { } auto callback = std::unique_ptr( - new FFICallback{.owner = lib, + new FFICallback{.owner = BaseObjectPtr(lib), .env = env, .thread_id = std::this_thread::get_id(), .fn = Global(isolate, fn), diff --git a/src/node_ffi.h b/src/node_ffi.h index 7758380138f..4b5df70124d 100644 --- a/src/node_ffi.h +++ b/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 owner; Environment* env; std::thread::id thread_id; v8::Global fn; @@ -161,6 +164,7 @@ class DynamicLibrary : public BaseObject { Environment* env, const std::string& name, const std::shared_ptr& fn); + static void CleanupHook(void* arg); static void CleanupFunctionInfo( const v8::WeakCallbackInfo& data); bool is_closed() const; diff --git a/test/ffi/test-ffi-callback-gc.js b/test/ffi/test-ffi-callback-gc.js new file mode 100644 index 00000000000..40ee6625ce6 --- /dev/null +++ b/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); +});