fix(napi): tokio runtime shutdown at exit by bmjhversteeg · Pull Request #3028 · napi-rs/napi-rs

bmjhversteeg · 2025-11-21T15:00:06Z

This is a proposal to fix #2970

I found that mmastrac/rust-ctor#304 (comment) and https://github.com/mmastrac/rust-ctor/wiki/FAQ:-Life%E2%80%90before-and-life%E2%80%90after-main#important-considerations, discourage using #[dtor] to cleanup tokio runtime, which might explain the issue.

Disclaimer: I'm not very familiar with napi-rs. I'm not sure why the second thread_cleanup() was added in #2850 and whether or not it was an integral part of fixing that particular issue.

Summary by CodeRabbit

Breaking Changes
- Removed public re-export of the ctor macro from the bindgen runtime module.
Refactor
- Simplified conditional compilation logic for module registration and thread cleanup.
- Updated thread cleanup function signature and implementation for improved C ABI compatibility.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Signed-off-by: Bart Versteeg <[email protected]>

graphite-app · 2025-11-21T15:00:13Z

How to use the Graphite Merge Queue

Add the label ready-to-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

coderabbitai · 2025-11-21T15:00:30Z

Walkthrough

Removed the public ctor macro re-export and refactored thread_cleanup from a conditional destructor function to an extern "C" function with explicit parameters. Unified conditional compilation under a single not(feature = "noop") gate and moved tokio/napi4-specific shutdown logic inside the function body.

Changes

Cohort / File(s)	Summary
Module registration refactoring `crates/napi/src/bindgen_runtime/mod.rs`	Removed public re-export of `ctor::ctor` macro.
Thread cleanup restructuring `crates/napi/src/bindgen_runtime/module_register.rs`	Changed `thread_cleanup` from a parameterless destructor function to an unsafe extern "C" function with (env, id, data) parameters. Unified conditional compilation under single `not(feature = "noop")` gate with nested `cfg(all(feature = "tokio_rt", feature = "napi4"))` guarding the shutdown logic. Simplified `napi_register_module_v1` conditional compilation path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The signature change for thread_cleanup from parameterless destructor to extern "C" function with C ABI parameters requires careful verification of compatibility with the NAPI callback registration mechanism.
Ensure the nested #[cfg(all(feature = "tokio_rt", feature = "napi4"))] guard correctly preserves the shutdown behavior for tokio runtime on supported platforms.
Verify that removing the destructor attribute and switching to extern "C" semantics doesn't introduce timing or ordering issues in module cleanup (particularly relevant to the Windows process exit regression context).

Poem

🐰 A cleanup so tidy, a function made clean,
No more lingering destructs in the in-between,
With C functions extern and conditions precise,
On Windows the process now exits so nice! ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(napi): tokio runtime shutdown at exit' directly describes the main change: fixing tokio runtime shutdown behavior at process exit on Windows, which aligns with the PR's core objective.
Linked Issues check	✅ Passed	The PR addresses the Windows process exit regression (issue #2970) by modifying thread_cleanup from a dtor macro-based function to a proper C-ABI function, fixing the tokio runtime shutdown issue reported.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the tokio runtime shutdown issue: removing the ctor re-export and restructuring thread_cleanup are both necessary for resolving the Windows exit hang regression.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a38ca7 and f120282.

📒 Files selected for processing (2)

crates/napi/src/bindgen_runtime/mod.rs (0 hunks)
crates/napi/src/bindgen_runtime/module_register.rs (2 hunks)

💤 Files with no reviewable changes (1)

crates/napi/src/bindgen_runtime/mod.rs

🔇 Additional comments (1)

crates/napi/src/bindgen_runtime/module_register.rs (1)

568-578: Sound approach: finalizer callback over destructor.

The refactoring from a destructor-based cleanup (using #[dtor]) to a finalizer callback registered via napi_wrap is a good architectural improvement. This aligns with the guidance from rust-ctor that discourages using destructors for resource cleanup, particularly for async runtimes on Windows where destructor ordering and DLL unloading can cause issues.

The finalizer approach ensures cleanup happens when N-API finalizes the exports object during shutdown, which is more predictable than relying on global destructor ordering.

Based on learnings

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Brooooooklyn · 2025-11-23T05:27:32Z

Using napi_wrap on exports to clean up the tokio runtime could lead to the following issue: #2847

Brooooooklyn · 2025-11-23T05:28:50Z

We can use this strategy on the wasm target because WebAssembly cannot use process.dlopen to load NAPI-RS modules.

fix(napi): tokio runtime shutdown at exit

f120282

Signed-off-by: Bart Versteeg <[email protected]>

bmjhversteeg mentioned this pull request Nov 21, 2025

fix(napi): shutdown runtime at env cleanup on windows #3026

Merged

bmjhversteeg closed this Nov 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(napi): tokio runtime shutdown at exit#3028

fix(napi): tokio runtime shutdown at exit#3028
bmjhversteeg wants to merge 1 commit intonapi-rs:mainfrom
bmjhversteeg:fix-tokio-shutdown-at-exit

bmjhversteeg commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

graphite-app bot commented Nov 21, 2025

Uh oh!

coderabbitai bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

Brooooooklyn commented Nov 23, 2025

Uh oh!

Brooooooklyn commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Comments

Conversation

bmjhversteeg commented Nov 21, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

graphite-app bot commented Nov 21, 2025

How to use the Graphite Merge Queue

Uh oh!

coderabbitai bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

Brooooooklyn commented Nov 23, 2025

Uh oh!

Brooooooklyn commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bmjhversteeg commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 21, 2025 •

edited

Loading