Skip to content

fix(napi): user_defined_rt can only be used once - #2841

Merged
Brooooooklyn merged 5 commits into
mainfrom
cursor/fix-napi-rs-issue-2833-0b22
Aug 7, 2025
Merged

Brooooooklyn merged 5 commits into
mainfrom
cursor/fix-napi-rs-issue-2833-0b22

Conversation

@Brooooooklyn

Copy link
Copy Markdown
Member

Fixes napi-rs async runtime restart by falling back to a default runtime after a user-defined runtime is consumed.

Previously, the user-defined Tokio runtime was take()n from its storage on first use, making it unavailable for subsequent restarts after shutdown. Since tokio::runtime::Runtime is not Clone, this change introduces a flag to track if a user-defined runtime was originally provided. If it was, and it's no longer available (i.e., already consumed and shut down), the system now gracefully falls back to creating a default Tokio runtime, preventing application crashes on restart.


Open in Cursor Open in Web

Co-authored-by: lynweklm <lynweklm@gmail.com>
@cursor

cursor Bot commented Aug 7, 2025

Copy link
Copy Markdown

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@graphite-app

graphite-app Bot commented Aug 7, 2025

Copy link
Copy Markdown

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.

@Brooooooklyn

Copy link
Copy Markdown
Member Author

@cursor fix clippy issue

@cursor

cursor Bot commented Aug 7, 2025

Copy link
Copy Markdown

I refactored the Tokio runtime creation logic in crates/napi/src/tokio_runtime.rs to enhance readability and conciseness. Specifically, I streamlined the conditional check for user-defined runtimes.

  • I modified the create_runtime function in crates/napi/src/tokio_runtime.rs.
  • I updated the conditional check for IS_USER_DEFINED_RT, replacing IS_USER_DEFINED_RT.get().copied().unwrap_or(false) with matches!(IS_USER_DEFINED_RT.get(), Some(true)). This change makes the code more idiomatic Rust when checking an Option<bool> value.

Open in Cursor Open in Web

Learn more about Cursor Agents

Co-authored-by: lynweklm <lynweklm@gmail.com>
@Brooooooklyn

Copy link
Copy Markdown
Member Author

@cursor Is there a race condition if multiple worker threads try to initialize a custom tokio runtime simultaneously?

@cursor

cursor Bot commented Aug 7, 2025

Copy link
Copy Markdown

I've been looking into the code you provided, specifically the start_async_runtime function and how OnceLock is used, by reading through the relevant files.

Based on my analysis, there is NO race condition when multiple worker threads try to initialize a custom Tokio runtime simultaneously. This is because OnceLock and LazyLock are used for USER_DEFINED_RT, IS_USER_DEFINED_RT, and RT, which guarantee that initialization happens exactly once, even with concurrent calls. Additionally, RwLock provides thread-safe access to the runtime.

While there's no race condition, I did notice a potential logical issue: if multiple threads attempt to set different custom runtimes, only the first one will succeed, and the others will be silently ignored. This isn't a race condition, but it might not be the intended behavior. You might consider documenting this behavior, adding a warning, or making it clear that subsequent calls to create_custom_tokio_runtime are no-ops.


Open in Cursor Open in Web

Learn more about Cursor Agents

@Brooooooklyn
Brooooooklyn marked this pull request as ready for review August 7, 2025 08:51
@Brooooooklyn Brooooooklyn changed the title Fix napi-rs issue 2833 fix(napi): user_defined_rt can only be used once Aug 7, 2025
@Brooooooklyn
Brooooooklyn requested a review from Copilot August 7, 2025 08:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where a user-defined Tokio runtime in napi-rs could only be used once before becoming unavailable for restarts. The fix introduces a flag to track when a user-defined runtime was originally provided and falls back to a default runtime when the user-defined one is no longer available.

  • Adds tracking of whether a user-defined runtime was originally provided
  • Modifies runtime creation logic to fall back to default runtime after user-defined runtime is consumed
  • Prevents application crashes on restart by ensuring a runtime is always available

Comment thread crates/napi/src/tokio_runtime.rs Outdated
Brooooooklyn and others added 2 commits August 7, 2025 16:54
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: lynweklm <lynweklm@gmail.com>
@Brooooooklyn

Copy link
Copy Markdown
Member Author

@cursoragent fix the compile error

@cursor

cursor Bot commented Aug 7, 2025

Copy link
Copy Markdown

I refined the logic for checking user-defined Tokio runtimes within the NAPI-RS crate. This change improves how the IS_USER_DEFINED_RT flag is accessed, ensuring more robust and idiomatic handling of its boolean value.

  • I modified the create_runtime function in crates/napi/src/tokio_runtime.rs.
  • I updated the access pattern for the IS_USER_DEFINED_RT OnceCell by adding .copied() after .get(). This converts Option<&bool> to Option<bool>, allowing unwrap_or(false) to operate directly on a bool value, which is a more idiomatic way to handle Copy types from an Option reference.

Open in Cursor Open in Web

Learn more about Cursor Agents

@Brooooooklyn
Brooooooklyn merged commit b997c4e into main Aug 7, 2025
64 checks passed
@Brooooooklyn
Brooooooklyn deleted the cursor/fix-napi-rs-issue-2833-0b22 branch August 7, 2025 09:58
This was referenced Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants