Skip to content

fix(android): synchronize ArgConverter's per-runtime long cache - #96

Open
NathanWalker wants to merge 1 commit into
android-react-nativefrom
fix/android-argconverter-cache-race
Open

NathanWalker wants to merge 1 commit into
android-react-nativefrom
fix/android-argconverter-cache-race

Conversation

@NathanWalker

Copy link
Copy Markdown
Contributor

Fixes #93.

Summary

ArgConverter::s_type_long_operations_cache is process-wide, but each runtime touches its own entry from its own thread:

  • insert: ArgConverter::Init → GetTypeLongCache(), when the runtime starts;
  • find: GetTypeLongCache(), for each long outside ±2^53 and in the NativeScriptLongNumber constructor;
  • erase: onDisposeEnv() / onDisposeRuntime(), when the runtime is disposed.

robin_hood's flat map moves entries when it rehashes or erases, so a worker that starts or stops can move another runtime's entry while that runtime is reading it.

-static robin_hood::unordered_map<napi_env, TypeLongOperationsCache *> s_type_long_operations_cache;
+static tns::ConcurrentMap<napi_env, TypeLongOperationsCache *> s_type_long_operations_cache;
  • It uses the same type and Get/Insert/Remove pattern as MetadataNode's per-runtime cache. Mirrored in the napi and jsi trees.
  • Only the owning runtime inserts or removes its own key, so get-then-insert is safe, and the returned pointer stays valid outside the lock.
  • Cost: an uncontended spin lock, taken only for longs outside ±2^53, for NativeScriptLongNumber construction, and at Init. Small longs never reach it.

Evidence

Emulator (API 36, arm64), V8. HWASan was enabled locally by uncommenting the -fsanitize=hwaddress lines in runtime/CMakeLists.txt.

  • Test suite: napi 522/522, jsi 450/450. No HWASan reports.
  • Stress test (not committed: it takes 90 s per run and depends on timing). 12 workers run at a time and are terminated after 20–300 ms. Each worker converts large longs (Long.parseLong, Long.MAX_VALUE, longValue()), allocates Java objects, and forces JS and Java GC. The main runtime forces GC every 25 ms. The runs include fix(android): deliver GC notifications under a per-runtime lock #91, so its GC-thread crash doesn't hide this one.
Code Runs Result
Before, napi 3 2 crashed with SIGSEGV in ArgConverter::ConvertFromJavaLong, reading a cache entry that held string bytes (",9007199")
Before, jsi 3 3 crashed in ConvertFromJavaLong; one was an HWASan tag-mismatch inside GetTypeLongCache
After, napi + jsi 6 no crash and no HWASan report over about 30,000 workers
  • Found by the same stress test, out of scope here:
    • jsi Console::s_rtToConsoleTimersMap races in createConsole. It crashed every jsi run until it was locked for the test.
    • JSR::env_to_jsr_cache in napi/v8/jsr.cpp.
    • Constants::APP_ROOT_FOLDER_PATH, which every worker writes in Runtime::Init.
    • The unlocked per-runtime maps MetadataNode::s_arrayObjects and the GlobalHelpers smart-stringify map.
  • Not verified: engines other than V8, and the React Native host-runtime path.

Merge Danger

Door: two-way

Blast Radius: Java long ↔ JS conversion for values outside ±2^53, on every Android runtime (main and workers, both binding layers)

It adds a spin lock around a map that had no synchronization. There is no new lock ordering, because the lock is never held while calling into JS or JNI.

s_type_long_operations_cache is process-wide, but each runtime inserts
its entry on init and erases it on dispose from its own thread. Make it a
tns::ConcurrentMap, as MetadataNode's per-runtime cache already is, in
both the napi and jsi trees.

Fixes #93
@coderabbitai

coderabbitai Bot commented Oct 25, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9de2a66f-b11f-465d-98fb-a88f49e118ed

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
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.

1 participant