refactor: introduce thread-safe RNFBHandleMap and migrate native listener registries repo-wide - #9093
refactor: introduce thread-safe RNFBHandleMap and migrate native listener registries repo-wide#9093mikehardy wants to merge 18 commits into
Conversation
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the thread-safety of Android functions streaming by synchronizing access to listener management and adjusting the cleanup logic to prevent lost events. Additionally, it enforces stricter quality standards by integrating a mandatory coverage evidence gate for native bridge and library changes, supported by updated documentation and new e2e test cases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new coverage_evidence gate to the validation and documentation workflow, detailing evidence requirements and anti-patterns. In the Android codebase, thread synchronization was added around functionsStreamingListeners to prevent race conditions. However, the review feedback correctly identifies that removing the listener cleanup from the success, error, and exception paths in NativeRNFBTurboFunctions introduces memory leaks, as completed or failed listeners are never removed from the static map. The reviewer provides actionable suggestions to safely remove these listeners from the map without triggering a cancellation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
c5187ce to
b04fb15
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9093 +/- ##
============================================
+ Coverage 68.36% 68.78% +0.42%
- Complexity 1914 2066 +152
============================================
Files 516 434 -82
Lines 37853 25115 -12738
Branches 5183 4217 -966
============================================
- Hits 25875 17273 -8602
+ Misses 10187 6493 -3694
+ Partials 1791 1349 -442
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hello 👋, this PR has been opened for more than 14 days with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
will come back to this shortly |
Synchronize access to the streaming listeners SparseArray and defer removal until explicit cancel or invalidate so final stream events are not dropped under concurrent unsubscribe. Add e2e coverage for native streaming errors and early stream cancel.
Add coverage_evidence_gate, mandatory Jacoco/lcov evidence package, NYC anti-pattern, and subagent return fields so native/lib bridge changes cannot close review without per-file coverage proof.
Replace unlocked SparseArray/NSMutableDictionary listener and transaction maps with HandleMap registries so snapshot and begin-transaction ids cannot race on JS vs SDK threads.
Replace unlocked SparseArray/NSMutableDictionary PENDING_TASKS with HandleMap so pause, resume, cancel, and complete cannot race on the same task id.
The H6 HandleMap wiring dropped a bracket so getQueryNamed never received its completion block and iOS builds failed.
Replace unlocked SparseArray/HashMap/NSMutableDictionary transaction and query listener maps with HandleMap so JS and SDK threads cannot race on the same id.
b04fb15 to
1229270
Compare
mikehardy
changed the title
|
Updated - finally ready to go on this odyssey please note - patch coverage is lower because I specifically accepted uncovered lines in the native code. I explored how to cover them but it required pulling in Firebase and other stuff during unit tests which wouldn't work well, or having bridge/helper intermediary objects which lowered cohesion just for some coverage. In the end, I chose to make sure the new handler object had 100% coverage but I accepted gaps in some areas of it's usage in all the packages. |
russellwheatley
left a comment
russellwheatley
left a comment
There was a problem hiding this comment.
Requesting changes on the storage cancel regression and the non-atomic replace pattern. The RNFBHandleMap approach is right, and cancelling outside the lock from the last round is resolved cleanly.
Blocking:
RNFBStorageTaskRegistry.takeAndCancelchanges whatStorageTask.cancel()returns to JS and orphans paused uploads.putReplacing/putOrSkip/putOrDiscardare alltakethenputoutside the lock, so replace is still a read-modify-write race. Two atomic primitives onRNFBHandleMapfix every call site at once.- Perf
putOrDiscard(id, trace, Trace::stop)reports the discarded trace instead of dropping it, andstopTracetakes the handle before the work that can throw.
Worth a decision, not blocking:
- Several call sites turned a silent overwrite into an exception reachable from JS input. Storage task ids come from a JS module counter that resets on reload while
PENDING_TASKSis static, so it is reachable in dev. - Messaging duplicate ids flipped to first-wins while the message store still takes the new message.
| public void put(K id, V handle) throws RNFBHandleCollisionException { | ||
| synchronized (lock) { | ||
| if (map.containsKey(id)) { | ||
| throw new RNFBHandleCollisionException(id); | ||
| } | ||
| map.put(id, handle); | ||
| } | ||
| } |
There was a problem hiding this comment.
Making put unique with no upsert pushes every replace call site into a non-atomic take then put: RNFBAuthCacheRegistry.putReplacing, RNFBFirestoreTransactionRegistry.putOrSkip, RNFBPerfHandleRegistry.putOrDiscard, RNFBStorageTaskRegistry.putOrDiscard, RNFBMessagingNotificationRegistry.putOrDiscard. Each of those is a read-modify-write outside the lock, which is the bug class this PR exists to remove.
Two more primitives here collapse all of them into one call:
| public void put(K id, V handle) throws RNFBHandleCollisionException { | |
| synchronized (lock) { | |
| if (map.containsKey(id)) { | |
| throw new RNFBHandleCollisionException(id); | |
| } | |
| map.put(id, handle); | |
| } | |
| } | |
| public void put(K id, V handle) throws RNFBHandleCollisionException { | |
| synchronized (lock) { | |
| if (map.containsKey(id)) { | |
| throw new RNFBHandleCollisionException(id); | |
| } | |
| map.put(id, handle); | |
| } | |
| } | |
| /** Stores {@code handle} only when {@code id} is free. Returns the existing handle, or null. */ | |
| public V putIfAbsent(K id, V handle) { | |
| synchronized (lock) { | |
| V existing = map.get(id); | |
| if (existing != null) { | |
| return existing; | |
| } | |
| map.put(id, handle); | |
| return null; | |
| } | |
| } | |
| /** Stores {@code handle} unconditionally. Returns the displaced handle, or null. */ | |
| public V putReplacing(K id, V handle) { | |
| synchronized (lock) { | |
| return map.put(id, handle); | |
| } | |
| } |
Callers still cancel or stop the returned handle after the call, so "the lock only moves pointers" still holds. Same on the iOS side.
There was a problem hiding this comment.
This upsert decision was one I struggled with - I like the idea of extending the API even though I was trying to keep it very small - these additional primitives allow upsert to work more cleanly for those that need it
👍
| /** Take then cancel outside the HandleMap lock. Returns {@code false} when no mapping existed. */ | ||
| boolean takeAndCancel(int taskId) { | ||
| StoragePendingHandle handle = map.take(taskId); | ||
| if (handle == null) { | ||
| return false; | ||
| } | ||
| handle.cancel(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This changes what JS gets back from StorageTask.cancel(). The old cancelTaskById returned storageTask.cancel(), and ReactNativeFirebaseStorageTask.cancel() only removes itself from the map inside the isInProgress() branch. Here the entry is taken unconditionally and the handle.cancel() result is thrown away, so:
cancel()resolvestruefor a task that was paused or not yet in progress, where it used to resolvefalse.- a paused upload is dropped from
PENDING_TASKSeven though nothing was cancelled, so no laterpause,resumeorcancelcan reach it. Same in the window wherestorageTaskis still null (the task registers itself in the constructor, before theStorageTaskis assigned), which leaves an upload running and untracked.
| /** Take then cancel outside the HandleMap lock. Returns {@code false} when no mapping existed. */ | |
| boolean takeAndCancel(int taskId) { | |
| StoragePendingHandle handle = map.take(taskId); | |
| if (handle == null) { | |
| return false; | |
| } | |
| handle.cancel(); | |
| return true; | |
| } | |
| /** Cancel outside the HandleMap lock. Returns the underlying cancel result. */ | |
| boolean takeAndCancel(int taskId) { | |
| StoragePendingHandle handle = map.get(taskId); | |
| if (handle == null) { | |
| return false; | |
| } | |
| // A successful cancel already removes the mapping via ReactNativeFirebaseStorageTask.destroyTask(). | |
| boolean cancelled = handle.cancel(); | |
| if (cancelled) { | |
| map.take(taskId); | |
| } | |
| return cancelled; | |
| } |
Worth an e2e for cancelling a paused upload, there is none today.
| try { | ||
| PENDING_TASKS.put(taskId, this); | ||
| } catch (RNFBHandleCollisionException collision) { | ||
| throw new IllegalStateException(collision.getMessage()); |
There was a problem hiding this comment.
Duplicate task ids used to overwrite silently, now they throw out of a constructor. TASK_ID in packages/storage/lib/StorageTask.ts is a module-level counter that resets to 0 on every JS reload, while PENDING_TASKS is static and outlives the JS context. Reload with an upload in flight and the next putFile throws from native.
With an atomic putReplacing on RNFBHandleMap this becomes one call: replace, then cancel the displaced task outside the lock. That keeps the old last-wins behavior without a JS-reachable throw.
| void putReplacing(String key, V value) { | ||
| map.take(key); | ||
| putOrDiscard(key, value); | ||
| } |
There was a problem hiding this comment.
take then putOrDiscard is not atomic. Two threads replacing the same key interleave as take, take, put, put, and the second value is silently dropped while the caller believes it stored. getSession and getJSError still hand the sessionId back to JS, so a later resolveMultiFactorSignIn looks up a resolver that was never cached.
| void putReplacing(String key, V value) { | |
| map.take(key); | |
| putOrDiscard(key, value); | |
| } | |
| void putReplacing(String key, V value) { | |
| map.putReplacing(key, value); | |
| } |
Needs the atomic putReplacing on RNFBHandleMap, see the comment there.
| if (!traces.putOrDiscard(id, trace, Trace::stop)) { | ||
| throw new IllegalStateException("perf trace id already registered: " + id); | ||
| } |
There was a problem hiding this comment.
Trace::stop is not a discard, it is how a trace gets reported to Firebase. On collision this sends a bogus near-zero-duration trace under the same name and then throws. Drop the handle instead. Same shape at lines 117, 143, 195, 226 and 243.
The throw is also reachable from JS input, startTrace used to overwrite and now rejects. With an atomic putReplacing you can keep the old behavior and stop the displaced handle outside the lock if you do want it recorded.
| return Tasks.call( | ||
| () -> { | ||
| Trace trace = traces.get(id); | ||
| Trace trace = traces.take(id); |
There was a problem hiding this comment.
The handle is taken before the attributes and metrics are applied. putAttribute throws on an invalid name, and if it does the trace is already out of the map so trace.stop() is never reached and nothing can stop or retry it. Before, the entry survived the failure. Use get for the work and take immediately before stop(). Same at 153, 201 and 249.
| // Unique put: duplicate message ids keep the first (no HashMap upsert). | ||
| if (remoteMessage.getNotification() != null) { | ||
| notifications.put(remoteMessage.getMessageId(), remoteMessage); | ||
| notifications.putOrDiscard(remoteMessage.getMessageId(), remoteMessage); |
There was a problem hiding this comment.
This flips duplicate message ids from last-wins to first-wins, and the storeFirebaseMessage call below always stores the new message, so the in-memory map and the store can now disagree for the same id. The map is static and only drained when a notification is opened, so a redelivered FCM message keeps the stale copy. putReplacing keeps the previous behavior.
| } | ||
|
|
||
| - (BOOL)put:(id)key value:(id)value error:(NSError **)error { | ||
| @synchronized(self) { |
There was a problem hiding this comment.
Non-blocking. The header tells callers never to @synchronized a RNFBHandleMap instance, but nothing enforces it and the Android side already uses a private lock object for exactly this reason. Locking on a private ivar makes the contract unbreakable rather than documented.
2 participants
Description
Native listener and handle maps were being read and written from more than one thread (JavaScript/TurboModule entry and Firebase SDK callbacks). That race can crash or drop events when maps such as
HashMap,SparseArray, orNSMutableDictionaryare updated without synchronization.A gap analysis identified the same class of problem in ~32 places (functions streaming listeners, firestore snapshot/transaction maps, database transaction/query maps, storage pending tasks, auth listeners and credential/MFA caches, app-check listeners, remote-config update handlers, perf Android trace/metric maps, messaging notification map).
The fix introduces a minimal shared
RNFBHandleMaphelper (Android + iOS): register handles under a lock withput/get/take/takeAll, then perform SDK cancel/remove and JS emit after the handle is taken out of the map. Package-specific registries wrap HandleMap where lifecycle needs extra rules (for example FunctionsStreamingHolder, Storage take-then-cancel, AuthputReplacingfor caches). One atomic commit per package.While hardening tests, we found Android unit tests were pinning Robolectric
sdkbecause of a subtle Gradle plugin interaction (DefaultSdkPickervs compile SDK). This PR switches pure JVM tests to JUnit where Robolectric is not required and drops unnecessary@Config(sdk=…)pins (see hygiene commits).This PR also starts in-package iOS XCTest infrastructure (
RNFBAppUnitTestsand per-package unit targets) so we can coverRNFBHandleMapand package registries at 100% on reachable native lines, including cases that full e2e does not exercise reliably.Related issues
streamimplementation forhttpsCallablefunctions #8210Related PRs
Release Summary
Fixes thread-safety crashes and listener races by routing native handle maps through a shared locked
RNFBHandleMapacross affected Firebase modules.Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/**/e2ejest/ JVM / XCTest unit tests added or updated where native lines are touchedTest Plan
yarn tests:android:unit) + Jacoco on touched registries/HandleMapyarn tests:ios:unit) + LCOV merge for HandleMap and package registriesgetE2eEmulatorHost()Internal note (maintainers only)
Linear tracking for merge automation only — not a public GitHub issue closure.
Fixes CPRN-365.