Skip to content

Conversation

paradowstack
Copy link
Contributor

@paradowstack paradowstack commented Aug 14, 2025

This pull request introduces a complete implementation of Node-API's thread-safe function support for the host runtime, including its registration, lifecycle management, and queueing semantics.

Thread-safe function implementation:

  • Implemented all core threadsafe related Node-API functions (napi_create_threadsafe_function, napi_call_threadsafe_function, napi_acquire_threadsafe_function, napi_release_threadsafe_function, napi_unref_threadsafe_function, napi_ref_threadsafe_function, napi_get_threadsafe_function_context) in RuntimeNodeApiAsync.cpp
  • Added a new ThreadSafeFunction class , providing a full implementation of Node-API's thread-safe function, including queueing, reference management, context retrieval, and finalization logic. This includes a global registry for safe handle management.

Testing and examples:


Note

Implements Node-API thread-safe functions with a new ThreadSafeFunction core, wires them into the runtime/injector, and adds test coverage with example integration.

  • Host Runtime (C++): Node-API thread-safe functions
    • Adds ThreadSafeFunction implementation (packages/host/cpp/ThreadsafeFunction.{hpp,cpp}) with queueing, ref/release, finalize, and context retrieval.
    • Exposes napi_* TSFN APIs in RuntimeNodeApiAsync.{hpp,cpp}: napi_create_threadsafe_function, napi_get_threadsafe_function_context, napi_call_threadsafe_function, napi_acquire_threadsafe_function, napi_release_threadsafe_function, napi_unref_threadsafe_function, napi_ref_threadsafe_function.
    • Registers new sources in Android build (packages/host/android/CMakeLists.txt).
    • Updates injector generator to export TSFN APIs (packages/host/scripts/generate-weak-node-api-injector.ts).
  • Tests/Examples
    • Adds TSFN test addon (packages/node-addon-examples/tests/threadsafe_function/*) and wires into examples index.
    • Increases test timeout for example cases (apps/test-app/App.tsx).
  • Release
    • Changeset: minor bump for react-native-node-api.

Written by Cursor Bugbot for commit 87a78f2. This will update automatically on new commits. Configure here.

@paradowstack paradowstack added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) labels Aug 14, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements complete thread-safe function support for Node-API, enabling cross-thread communication between native code and JavaScript. The implementation provides all core Node-API threadsafe functions with proper lifecycle management, queueing semantics, and reference counting.

Key changes:

  • Added ThreadSafeFunction class with full Node-API compatibility including creation, calling, acquire/release, and ref/unref operations
  • Implemented all seven Node-API threadsafe functions in RuntimeNodeApiAsync.cpp with proper error handling and state management
  • Added comprehensive test suite based on Node.js official tests to validate functionality across different threading scenarios

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/host/cpp/ThreadsafeFunction.hpp Header defining the ThreadSafeFunction class with thread-safe queue management and lifecycle controls
packages/host/cpp/ThreadsafeFunction.cpp Complete implementation of thread-safe function with global registry, queueing, and finalization logic
packages/host/cpp/RuntimeNodeApiAsync.cpp Node-API function implementations delegating to ThreadSafeFunction class methods
packages/host/scripts/generate-weak-node-api-injector.ts Added threadsafe function names to implemented functions list
packages/node-addon-examples/tests/threadsafe_function/* Test suite files including C++ addon and JavaScript test harness
packages/host/android/CMakeLists.txt Build configuration updates to include new ThreadsafeFunction source files

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// Global registry to map unique IDs to ThreadSafeFunction instances.
// We use IDs instead of raw pointers to avoid any use-after-free issues.
static std::unordered_map<std::uintptr_t,
std::shared_ptr<callstack::nodeapihost::ThreadSafeFunction>>
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

The static global registry introduces potential memory management complexity. Consider using a singleton pattern or RAII wrapper to ensure proper cleanup on module unload.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleanup is ensured programatically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could / should this map be owned by the napi_env?

Copy link
Contributor Author

@paradowstack paradowstack Aug 18, 2025

Choose a reason for hiding this comment

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

My understanding is that it would change anything:

  1. Functions are called and referenced using handle napi_threadsafe_function, not using napi_env. Threadsafe function will always use the correct env, the one the function was created for.
  2. I am not aware of any way to detect and cleanup the map when napi_env is destroyed - no change in this aspect.
  3. There will be no memory leaks since smart pointers are used anyway.

I just cannot see benefits from adding this "ownership" layer, but I may be missing something. Could you see any?

Copy link
Collaborator

Choose a reason for hiding this comment

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

that it would change anything

That it would or that it would not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be would not

job->state = AsyncJob::State::Cancelled;
return napi_ok;
}

Copy link
Collaborator

@kraenhansen kraenhansen Aug 18, 2025

Choose a reason for hiding this comment

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

I wonder if the name of this file needs an update now that it's not "just async work related"? Or of this should go into as separate file? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about that, finally I decided to put it there since the threadsafe functions in Node-API are called asynchronously and other implementation often group them together. But I am open to change that I move it to the separate file.

Copy link
Collaborator

@kraenhansen kraenhansen Aug 18, 2025

Choose a reason for hiding this comment

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

I don't have a strong preference either way 👍
If it's a conscious choice, I'm all for that. Just didn't seem that related to me.

Copy link
Collaborator

@shirakaba shirakaba left a comment

Choose a reason for hiding this comment

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

Just did a pair-programming session with Kræn and came up with one important change for thread-hopping!

Comment on lines 142 to 144
// Hop to JS thread; we drain one item per hop to keep latency predictable
// and avoid long monopolization of the JS queue.
invoker->invokeAsync([self = shared_from_this()] { self->processQueue(); });
Copy link
Collaborator

@shirakaba shirakaba Sep 26, 2025

Choose a reason for hiding this comment

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

This one-line change was enough to get thread-hopping in NativeScript Node-API working (where before, it was crashing):

Suggested change
// Hop to JS thread; we drain one item per hop to keep latency predictable
// and avoid long monopolization of the JS queue.
invoker->invokeAsync([self = shared_from_this()] { self->processQueue(); });
// Invoke from the current thread. Libraries like NativeScript can wrap a
// JS function in an Objective-C block to be dispatched onto another thread
// (e.g. the main thread, with the intention of accessing the UI), and so we
// should run the JS function on the same thread the Objective-C block was
// dispatched to.
invoker->invokeSync([self = shared_from_this()] { self->processQueue(); });

I was able to do intricate moves back and forth between the JS and main threads, and use JS timers (see screenshot below). It all worked perfectly.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just came back from the long vacation and saw that, sweet!
I tried that with my reproduction of the problem but it didn't help - looks like my repro conditions were not correct.
I have applied the changes 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this change introduced problems to the solution, such as deadlocks, some of them are already fixed, but I will have to revisit the entire PR in the future.

@paradowstack paradowstack force-pushed the kp/threadsafe-functions branch from 3239f2b to b9ebf1f Compare September 30, 2025 06:18
@paradowstack paradowstack marked this pull request as ready for review September 30, 2025 06:18
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@paradowstack paradowstack marked this pull request as draft October 2, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants