Skip to content

Commit aa6ee7d

Browse files
bartlomiejuclaude
andauthored
fix(ext/napi): call wrap/ref finalizers at shutdown (#32592)
- Track NAPI finalizer callbacks (from `napi_wrap`, `napi_create_external`, `napi_add_finalizer`, `napi_set_instance_data`) in a `PendingNapiFinalizer` list shared between `NapiState` and `Env` - Call tracked finalizers in LIFO order during `MainWorker` shutdown while V8 is still alive with a proper `HandleScope`, matching Node.js's `RefTracker::FinalizeAll()` behavior - When finalizers fire naturally via GC weak callbacks, they deregister from the tracker to avoid double-calling This fixes native addons like DuckDB that rely on C++ destructor cleanup (e.g. WAL checkpointing) during process exit. Previously these finalizers only ran via V8 GC weak callbacks, which never fire for objects still reachable at exit. Closes #27468 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 5b9986d commit aa6ee7d

File tree

10 files changed

+224
-4
lines changed

10 files changed

+224
-4
lines changed

cli/lib/worker.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -853,6 +853,11 @@ impl LibMainWorker {
853853
self.worker.dispatch_process_exit_event()
854854
}
855855

856+
#[inline]
857+
pub fn run_napi_ref_finalizers(&mut self) {
858+
self.worker.run_napi_ref_finalizers()
859+
}
860+
856861
pub async fn execute_main_module(&mut self) -> Result<(), CoreError> {
857862
let id = self.worker.preload_main_module(&self.main_module).await?;
858863
self.worker.evaluate_module(id).await
@@ -905,6 +910,7 @@ impl LibMainWorker {
905910

906911
self.worker.dispatch_unload_event()?;
907912
self.worker.dispatch_process_exit_event()?;
913+
self.worker.run_napi_ref_finalizers();
908914

909915
Ok(self.worker.exit_code())
910916
}

cli/tools/bench/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,7 @@ async fn bench_specifier_inner(
291291
worker
292292
.dispatch_process_exit_event()
293293
.map_err(|e| CoreErrorKind::Js(e).into_box())?;
294+
worker.run_napi_ref_finalizers();
294295

295296
// Ensure the worker has settled so we can catch any remaining unhandled rejections. We don't
296297
// want to wait forever here.

cli/worker.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ impl CliMainWorker {
159159

160160
self.worker.dispatch_unload_event()?;
161161
self.worker.dispatch_process_exit_event()?;
162+
self.worker.run_napi_ref_finalizers();
162163

163164
if let Some(mut coverage_collector) = coverage_cell.borrow_mut().take() {
164165
coverage_collector.stop_collecting()?;
@@ -217,6 +218,7 @@ impl CliMainWorker {
217218

218219
self.inner.worker.dispatch_unload_event()?;
219220
self.inner.worker.dispatch_process_exit_event()?;
221+
self.inner.worker.run_napi_ref_finalizers();
220222

221223
Ok(())
222224
}

ext/napi/js_native_api.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,9 @@ impl Reference {
132132
// it might free the reference (which would be a UAF)
133133
let ownership = reference.ownership;
134134
if let Some(finalize_cb) = finalize_cb {
135+
// Deregister before calling so it won't be called again at shutdown
136+
let env = unsafe { &*reference.env };
137+
env.remove_ref_finalizer(finalize_data);
135138
unsafe {
136139
finalize_cb(reference.env as _, finalize_data, finalize_hint);
137140
}
@@ -2520,6 +2523,15 @@ fn napi_wrap(
25202523
finalize_hint,
25212524
);
25222525

2526+
if let Some(cb) = finalize_cb {
2527+
env.add_ref_finalizer(
2528+
env_ptr as napi_env,
2529+
cb,
2530+
native_object,
2531+
finalize_hint,
2532+
);
2533+
}
2534+
25232535
let reference = Reference::into_raw(reference) as *mut c_void;
25242536

25252537
if !result.is_null() {
@@ -2571,6 +2583,7 @@ fn unwrap(
25712583
}
25722584

25732585
if !keep {
2586+
env.remove_ref_finalizer(reference.finalize_data);
25742587
assert!(obj.delete_private(scope, napi_wrap).unwrap_or(false));
25752588
unsafe { Reference::remove(reference) };
25762589
}
@@ -2622,6 +2635,12 @@ fn napi_create_external<'s>(
26222635
let external = v8::External::new(scope, wrapper as _);
26232636

26242637
if let Some(finalize_cb) = finalize_cb {
2638+
env.add_ref_finalizer(
2639+
env_ptr as napi_env,
2640+
finalize_cb,
2641+
data,
2642+
finalize_hint,
2643+
);
26252644
Reference::into_raw(Reference::new(
26262645
env_ptr,
26272646
external.into(),
@@ -3669,6 +3688,16 @@ fn napi_add_finalizer(
36693688
} else {
36703689
ReferenceOwnership::Userland
36713690
};
3691+
3692+
if let Some(cb) = finalize_cb {
3693+
env.add_ref_finalizer(
3694+
env_ptr as napi_env,
3695+
cb,
3696+
finalize_data,
3697+
finalize_hint,
3698+
);
3699+
}
3700+
36723701
let reference = Reference::new(
36733702
env,
36743703
value.into(),
@@ -3725,6 +3754,9 @@ fn napi_set_instance_data(
37253754
) -> napi_status {
37263755
let env = check_env!(env);
37273756

3757+
// Note: instance data finalizers are NOT registered in ref_tracker because
3758+
// they already have their own teardown path in NapiState::Drop which calls
3759+
// EnvShared instance_data finalize_cb directly.
37283760
env.shared_mut().instance_data = Some(InstanceData {
37293761
data,
37303762
finalize_cb,

ext/napi/lib.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,9 +303,22 @@ pub struct napi_node_version {
303303
pub trait PendingNapiAsyncWork: FnOnce() + Send + 'static {}
304304
impl<T> PendingNapiAsyncWork for T where T: FnOnce() + Send + 'static {}
305305

306+
/// A pending NAPI finalizer callback to be called at environment shutdown.
307+
/// This matches Node.js's behavior of tracking references with finalize
308+
/// callbacks and calling them during `napi_env::DeleteMe()`.
309+
pub struct PendingNapiFinalizer {
310+
pub env: napi_env,
311+
pub cb: napi_finalize,
312+
pub data: *mut c_void,
313+
pub hint: *mut c_void,
314+
}
315+
306316
pub struct NapiState {
307317
// Thread safe functions.
308318
pub env_cleanup_hooks: Rc<RefCell<Vec<(napi_cleanup_hook, *mut c_void)>>>,
319+
/// Tracked finalizer callbacks that should be called at shutdown.
320+
/// Matches Node.js `finalizing_reflist` / `reflist` behavior.
321+
pub ref_tracker: Rc<RefCell<Vec<PendingNapiFinalizer>>>,
309322
pub env_shared_ptrs: Vec<*mut EnvShared>,
310323
}
311324

@@ -345,6 +358,10 @@ impl Drop for NapiState {
345358
}
346359
}
347360

361+
// Note: remaining ref tracker entries are not finalized here because
362+
// V8 is no longer alive. MainWorker::run_napi_ref_finalizers() handles
363+
// this by calling finalizers directly while V8 is still alive.
364+
348365
// Call instance data finalize callbacks for all registered EnvShared instances.
349366
// Each entry should be unique since each op_napi_open creates a fresh EnvShared.
350367
debug_assert!(
@@ -418,6 +435,7 @@ pub struct Env {
418435
pub shared: *mut EnvShared,
419436
pub async_work_sender: V8CrossThreadTaskSpawner,
420437
cleanup_hooks: Rc<RefCell<Vec<(napi_cleanup_hook, *mut c_void)>>>,
438+
ref_tracker: Rc<RefCell<Vec<PendingNapiFinalizer>>>,
421439
external_ops_tracker: ExternalOpsTracker,
422440
pub last_error: napi_extended_error_info,
423441
pub last_exception: Option<v8::Global<v8::Value>>,
@@ -439,6 +457,7 @@ impl Env {
439457
report_error: v8::Global<v8::Function>,
440458
sender: V8CrossThreadTaskSpawner,
441459
cleanup_hooks: Rc<RefCell<Vec<(napi_cleanup_hook, *mut c_void)>>>,
460+
ref_tracker: Rc<RefCell<Vec<PendingNapiFinalizer>>>,
442461
external_ops_tracker: ExternalOpsTracker,
443462
) -> Self {
444463
Self {
@@ -451,6 +470,7 @@ impl Env {
451470
open_handle_scopes: 0,
452471
async_work_sender: sender,
453472
cleanup_hooks,
473+
ref_tracker,
454474
external_ops_tracker,
455475
last_error: napi_extended_error_info {
456476
error_message: std::ptr::null(),
@@ -533,6 +553,28 @@ impl Env {
533553
None => panic!("Cannot remove cleanup hook which was not registered"),
534554
}
535555
}
556+
557+
pub fn add_ref_finalizer(
558+
&self,
559+
env: napi_env,
560+
cb: napi_finalize,
561+
data: *mut c_void,
562+
hint: *mut c_void,
563+
) {
564+
self.ref_tracker.borrow_mut().push(PendingNapiFinalizer {
565+
env,
566+
cb,
567+
data,
568+
hint,
569+
});
570+
}
571+
572+
pub fn remove_ref_finalizer(&self, data: *mut c_void) {
573+
let mut tracker = self.ref_tracker.borrow_mut();
574+
if let Some(pos) = tracker.iter().rposition(|f| f.data == data) {
575+
tracker.remove(pos);
576+
}
577+
}
536578
}
537579

538580
deno_core::extension!(deno_napi,
@@ -545,6 +587,7 @@ deno_core::extension!(deno_napi,
545587
state = |state, options| {
546588
state.put(NapiState {
547589
env_cleanup_hooks: Rc::new(RefCell::new(vec![])),
590+
ref_tracker: Rc::new(RefCell::new(vec![])),
548591
env_shared_ptrs: vec![],
549592
});
550593
if let Some(loader) = options.deno_rt_native_addon_loader {
@@ -578,6 +621,7 @@ fn op_napi_open<'scope>(
578621
let (
579622
async_work_sender,
580623
cleanup_hooks,
624+
ref_tracker,
581625
external_ops_tracker,
582626
deno_rt_native_addon_loader,
583627
path,
@@ -590,6 +634,7 @@ fn op_napi_open<'scope>(
590634
(
591635
op_state.borrow::<V8CrossThreadTaskSpawner>().clone(),
592636
napi_state.env_cleanup_hooks.clone(),
637+
napi_state.ref_tracker.clone(),
593638
op_state.external_ops_tracker.clone(),
594639
op_state.try_borrow::<DenoRtNativeAddonLoaderRc>().cloned(),
595640
path,
@@ -618,6 +663,7 @@ fn op_napi_open<'scope>(
618663
v8::Global::new(scope, report_error),
619664
async_work_sender,
620665
cleanup_hooks,
666+
ref_tracker,
621667
external_ops_tracker,
622668
);
623669
env.shared = Box::into_raw(Box::new(env_shared));

runtime/worker.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -984,6 +984,42 @@ impl MainWorker {
984984
Ok(())
985985
}
986986

987+
/// Runs pending NAPI ref/wrap finalizers by calling them directly while V8
988+
/// is still alive. This matches Node.js behavior where `napi_env::DeleteMe()`
989+
/// calls `RefTracker::FinalizeAll()` during environment teardown.
990+
pub fn run_napi_ref_finalizers(&mut self) {
991+
let finalizers = {
992+
let op_state = self.js_runtime.op_state();
993+
let op_state = op_state.borrow();
994+
match op_state.try_borrow::<deno_napi::NapiState>() {
995+
Some(s) => {
996+
let mut tracker = s.ref_tracker.borrow_mut();
997+
std::mem::take(&mut *tracker)
998+
}
999+
None => return,
1000+
}
1001+
};
1002+
if finalizers.is_empty() {
1003+
return;
1004+
}
1005+
1006+
// Call each finalizer in reverse (LIFO) order within a proper HandleScope
1007+
// so native addon code can use NAPI functions during cleanup. LIFO matches
1008+
// Node.js's linked list behavior (head insertion → reverse iteration).
1009+
{
1010+
deno_core::scope!(scope, &mut self.js_runtime);
1011+
let _scope = v8::HandleScope::new(scope);
1012+
for f in finalizers.into_iter().rev() {
1013+
// SAFETY: The pointers (env, data, hint) were stored when the native
1014+
// addon called napi_wrap/napi_create_external/napi_add_finalizer and
1015+
// V8 is still alive (we have an active HandleScope).
1016+
unsafe {
1017+
(f.cb)(f.env, f.data, f.hint);
1018+
}
1019+
}
1020+
}
1021+
}
1022+
9871023
/// Dispatches "beforeunload" event to the JavaScript runtime. Returns a boolean
9881024
/// indicating if the event was prevented and thus event loop should continue
9891025
/// running.

tests/integration/napi_tests.rs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,3 +98,40 @@ fn napi_tests() {
9898
}
9999
assert!(output.status.success());
100100
}
101+
102+
/// Test that NAPI wrap finalizers are called at shutdown even when the
103+
/// wrapped JS object is still reachable. This matches Node.js behavior
104+
/// and is required for native addons like DuckDB that rely on destructor
105+
/// cleanup (e.g., WAL checkpointing) during process exit.
106+
#[test_util::test]
107+
fn napi_wrap_leak_pointers_finalizer_on_shutdown() {
108+
napi_build();
109+
110+
let output = deno_cmd()
111+
.current_dir(napi_tests_path())
112+
.arg("run")
113+
.arg("--allow-read")
114+
.arg("--allow-env")
115+
.arg("--allow-ffi")
116+
.arg("--config")
117+
.arg(deno_config_path())
118+
.arg("--no-lock")
119+
.arg("wrap_leak.js")
120+
.envs(env_vars_for_npm_tests())
121+
.output()
122+
.unwrap();
123+
let stdout = std::str::from_utf8(&output.stdout).unwrap();
124+
let stderr = std::str::from_utf8(&output.stderr).unwrap();
125+
126+
if !output.status.success() {
127+
eprintln!("exit code {:?}", output.status.code());
128+
println!("stdout {}", stdout);
129+
println!("stderr {}", stderr);
130+
}
131+
assert!(output.status.success());
132+
assert!(
133+
stdout.contains("pointers released on shutdown"),
134+
"Expected wrap finalizer to run at shutdown, got stdout: {}",
135+
stdout
136+
);
137+
}

tests/napi/instance_data_test.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@ if (import.meta.main) {
2727
assertEquals(code, 0);
2828

2929
const stdoutText = new TextDecoder().decode(stdout);
30-
const stdoutLines = stdoutText.split("\n");
31-
assertEquals(stdoutLines.length, 3);
32-
assertEquals(stdoutLines[0], "set instance data");
33-
assertEquals(stdoutLines[1], "instance_data_free(42)");
30+
assertEquals(
31+
stdoutText,
32+
"set instance data\ninstance_data_free(42)\n",
33+
);
3434
});
3535
}

tests/napi/src/finalizer.rs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,48 @@ extern "C" fn test_external_arraybuffer(
144144
result
145145
}
146146

147+
/// Wrap finalizer that prints a message. Used to test that wrap finalizers
148+
/// are called at shutdown even when the wrapped object is still reachable.
149+
unsafe extern "C" fn wrap_leak_release(
150+
_env: napi_env,
151+
data: *mut ::std::os::raw::c_void,
152+
_hint: *mut ::std::os::raw::c_void,
153+
) {
154+
let msg = unsafe { Box::from_raw(data as *mut String) };
155+
println!("{}", msg);
156+
}
157+
158+
/// Creates a napi_wrap on the given JS object with a finalizer that prints
159+
/// a message. The wrap is NOT explicitly removed, so only the shutdown
160+
/// finalizer path will trigger the callback. This tests that NAPI wrap
161+
/// finalizers are called during environment teardown (matching Node.js
162+
/// behavior).
163+
extern "C" fn test_wrap_leak(
164+
env: napi_env,
165+
info: napi_callback_info,
166+
) -> napi_value {
167+
let (args, argc, _) = napi_get_callback_info!(env, info, 1);
168+
assert_eq!(argc, 1);
169+
170+
let mut ty = -1;
171+
assert_napi_ok!(napi_typeof(env, args[0], &mut ty));
172+
assert_eq!(ty, napi_object);
173+
174+
let msg = Box::new(String::from("pointers released on shutdown"));
175+
let msg_ptr = Box::into_raw(msg) as *mut ::std::os::raw::c_void;
176+
177+
assert_napi_ok!(napi_wrap(
178+
env,
179+
args[0],
180+
msg_ptr,
181+
Some(wrap_leak_release),
182+
ptr::null_mut(),
183+
ptr::null_mut(),
184+
));
185+
186+
args[0]
187+
}
188+
147189
pub fn init(env: napi_env, exports: napi_value) {
148190
let properties = &[
149191
napi_new_property!(env, "test_bind_finalizer", test_bind_finalizer),
@@ -159,6 +201,7 @@ pub fn init(env: napi_env, exports: napi_value) {
159201
"test_static_external_buffer",
160202
test_static_external_buffer
161203
),
204+
napi_new_property!(env, "test_wrap_leak", test_wrap_leak),
162205
];
163206

164207
assert_napi_ok!(napi_define_properties(

0 commit comments

Comments
 (0)