fix(android,ios): add thread safety to listener operations#3152
fix(android,ios): add thread safety to listener operations#3152
Conversation
- Android: wrap listener add/remove/iterate in synchronized blocks to prevent ConcurrentModificationException - iOS: add warning logs when HybridRnIap is deallocated during purchase/error listener callbacks for better diagnostics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @hyochan, 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 significantly enhances the stability and diagnostic capabilities of the in-app purchase (IAP) module across Android and iOS. It addresses potential concurrency issues on Android by ensuring listener operations are thread-safe, and on iOS, it provides better visibility into cases where purchase events might be silently missed due to the native module's lifecycle, ultimately leading to a more robust IAP experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 effectively addresses thread safety in the Android listener management by introducing synchronized blocks, which should prevent ConcurrentModificationException. The diagnostic logging added to the iOS implementation is also a valuable improvement for debugging deallocated listener issues.
However, I've identified a significant issue in the Android listener removal logic. The removePurchaseUpdatedListener and removePurchaseErrorListener methods incorrectly clear all listeners instead of just the specified one. I've left detailed comments with suggestions to fix this to ensure correct behavior and consistency with other listener removal methods in the class.
| // Note: Kotlin doesn't have easy closure comparison, so we'll clear all listeners | ||
| purchaseUpdatedListeners.clear() | ||
| synchronized(purchaseUpdatedListeners) { | ||
| purchaseUpdatedListeners.clear() |
There was a problem hiding this comment.
Using purchaseUpdatedListeners.clear() here will remove all registered listeners, not just the one provided. This can lead to subtle bugs where one part of the application inadvertently removes listeners from another part.
This is inconsistent with other listener removal methods in this class, such as removePromotedProductListenerIOS and removeUserChoiceBillingListenerAndroid, which correctly use remove(listener).
To fix this and ensure only the specific listener is removed, please change clear() to remove(listener).
| purchaseUpdatedListeners.clear() | |
| purchaseUpdatedListeners.remove(listener) |
There was a problem hiding this comment.
Fixed in a7ae01d. Changed clear() to remove(listener) for targeted removal.
| // Note: Kotlin doesn't have easy closure comparison, so we'll clear all listeners | ||
| purchaseErrorListeners.clear() | ||
| synchronized(purchaseErrorListeners) { | ||
| purchaseErrorListeners.clear() |
There was a problem hiding this comment.
Similar to removePurchaseUpdatedListener, this method uses purchaseErrorListeners.clear(), which will remove all error listeners. This can cause other parts of the app to miss purchase error events.
Please change this to purchaseErrorListeners.remove(listener) to ensure only the specified listener is removed.
| purchaseErrorListeners.clear() | |
| purchaseErrorListeners.remove(listener) |
There was a problem hiding this comment.
Fixed in a7ae01d. Changed to remove(listener) to match the targeted removal pattern.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds thread-safety around Android listener management, adds explicit deallocation guards with warning logs for iOS listener closures, and increases the CI timeout for the iOS validation job from 30 to 60 minutes. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt (1)
782-797: Invoking callbacks inside the synchronized block risks aConcurrentModificationExceptionon same-thread reentrance.Java
synchronizedis reentrant per thread. If a listener callback synchronously callsremovePurchaseUpdatedListener(which callspurchaseUpdatedListeners.clear()), the clear happens whileArrayList.forEachis still iterating the same list, throwing aConcurrentModificationException. The safe idiom is to snapshot the list under the lock and iterate outside it:♻️ Proposed snapshot-before-iterate pattern
private fun sendPurchaseUpdate(purchase: NitroPurchase) { RnIapLog.result( "sendPurchaseUpdate", mapOf("productId" to purchase.productId, "platform" to purchase.platform) ) - synchronized(purchaseUpdatedListeners) { - purchaseUpdatedListeners.forEach { it(purchase) } - } + val snapshot = synchronized(purchaseUpdatedListeners) { ArrayList(purchaseUpdatedListeners) } + snapshot.forEach { it(purchase) } } private fun sendPurchaseError(error: NitroPurchaseResult) { RnIapLog.result( "sendPurchaseError", mapOf("code" to error.code, "message" to error.message) ) - synchronized(purchaseErrorListeners) { - purchaseErrorListeners.forEach { it(error) } - } + val snapshot = synchronized(purchaseErrorListeners) { ArrayList(purchaseErrorListeners) } + snapshot.forEach { it(error) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt` around lines 782 - 797, The synchronized blocks in the listener dispatch methods (e.g., where purchaseUpdatedListeners and purchaseErrorListeners are iterated in sendPurchaseUpdated/sendPurchaseError) call callbacks while holding the lock, risking ConcurrentModificationException if a callback removes listeners (e.g., removePurchaseUpdatedListener clears the list). Fix by copying/snapshotting the listener list inside the synchronized block (e.g., create a new List from purchaseUpdatedListeners or purchaseErrorListeners) and then iterate over that snapshot outside the synchronized block so callbacks run without holding the lock.ios/HybridRnIap.swift (1)
923-926: Consider adding a warning log to thepromotedProductSubguard for consistency.The
purchaseUpdatedSubandpurchaseErrorSubclosures now log a warning on deallocation (lines 903 and 924), but thepromotedProductSubguard at line 944 silently drops the event:guard let self else { return }Since this PR's intent is to improve diagnostics for dropped events (issue
#3150), applying the same pattern here would make deallocation-related drops uniformly observable.♻️ Proposed consistency fix for `promotedProductSub`
- guard let self else { return } + guard let self else { + RnIapLog.warn("promotedProductListenerIOS: HybridRnIap deallocated, promoted product event dropped") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/HybridRnIap.swift` around lines 923 - 926, The promotedProductSub closure currently uses a silent guard (guard let self else { return }) which drops events without a diagnostic; update the guard in the promotedProductSub closure to mirror purchaseUpdatedSub and purchaseErrorSub by logging a warning before returning (e.g., RnIapLog.warn("promotedProductSub: HybridRnIap deallocated, event dropped")), ensuring the deallocation case in promotedProductSub is observable; locate the promotedProductSub closure and replace the silent guard with the guarded self plus RnIapLog.warn call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt`:
- Around line 734-755: endConnection() currently calls
purchaseUpdatedListeners.clear() and purchaseErrorListeners.clear() without
holding the same locks used during dispatch, which can race with
sendPurchaseUpdate/sendPurchaseError; fix by wrapping both clear() calls inside
synchronized(purchaseUpdatedListeners) and synchronized(purchaseErrorListeners)
respectively (i.e., acquire the same monitors used by add/remove and by
sendPurchaseUpdate/sendPurchaseError) so teardown cannot clear the lists while
they are being iterated.
---
Nitpick comments:
In `@android/src/main/java/com/margelo/nitro/iap/HybridRnIap.kt`:
- Around line 782-797: The synchronized blocks in the listener dispatch methods
(e.g., where purchaseUpdatedListeners and purchaseErrorListeners are iterated in
sendPurchaseUpdated/sendPurchaseError) call callbacks while holding the lock,
risking ConcurrentModificationException if a callback removes listeners (e.g.,
removePurchaseUpdatedListener clears the list). Fix by copying/snapshotting the
listener list inside the synchronized block (e.g., create a new List from
purchaseUpdatedListeners or purchaseErrorListeners) and then iterate over that
snapshot outside the synchronized block so callbacks run without holding the
lock.
In `@ios/HybridRnIap.swift`:
- Around line 923-926: The promotedProductSub closure currently uses a silent
guard (guard let self else { return }) which drops events without a diagnostic;
update the guard in the promotedProductSub closure to mirror purchaseUpdatedSub
and purchaseErrorSub by logging a warning before returning (e.g.,
RnIapLog.warn("promotedProductSub: HybridRnIap deallocated, event dropped")),
ensuring the deallocation case in promotedProductSub is observable; locate the
promotedProductSub closure and replace the silent guard with the guarded self
plus RnIapLog.warn call.
Android: - Use remove(listener) instead of clear() for targeted removal - Use snapshot-before-iterate pattern to prevent reentrance issues - Add synchronized blocks to endConnection() clear() calls iOS: - Add warning log to promotedProductSub guard for consistency
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ios/HybridRnIap.swift (1)
876-894: iOS listener arrays need synchronization protection against concurrent mutations and iterations.
purchaseUpdatedListeners,purchaseErrorListeners, andpromotedProductListenersare mutated by add/remove methods (which run on the JS runtime thread per Nitro's dispatcher model) and iterated insideMainActorclosures, creating a race condition when mutations and iterations overlap. Android implements this correctly withsynchronizedblocks and snapshot-based iteration; iOS lacks this protection. Since the PR scopes iOS to diagnostics only, this is appropriate to address in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/HybridRnIap.swift` around lines 876 - 894, The listener arrays purchaseUpdatedListeners, purchaseErrorListeners, and promotedProductListeners are not thread-safe: addPurchaseUpdatedListener / addPurchaseErrorListener and the removePurchase... methods mutate them on the JS runtime thread while they are iterated inside MainActor closures, causing races; fix by introducing a single dedicated synchronization primitive (e.g., a private serial DispatchQueue or NSLock or a Swift actor) guarding all accesses to these arrays, update addPurchaseUpdatedListener, addPurchaseErrorListener, removePurchaseUpdatedListener, removePurchaseErrorListener to perform mutations on that lock/queue, and change any iteration sites to take a snapshot under the same lock (copy the array while holding the lock, then iterate the copy on MainActor) so iterations do not race with concurrent mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ios/HybridRnIap.swift`:
- Around line 876-894: The listener arrays purchaseUpdatedListeners,
purchaseErrorListeners, and promotedProductListeners are not thread-safe:
addPurchaseUpdatedListener / addPurchaseErrorListener and the removePurchase...
methods mutate them on the JS runtime thread while they are iterated inside
MainActor closures, causing races; fix by introducing a single dedicated
synchronization primitive (e.g., a private serial DispatchQueue or NSLock or a
Swift actor) guarding all accesses to these arrays, update
addPurchaseUpdatedListener, addPurchaseErrorListener,
removePurchaseUpdatedListener, removePurchaseErrorListener to perform mutations
on that lock/queue, and change any iteration sites to take a snapshot under the
same lock (copy the array while holding the lock, then iterate the copy on
MainActor) so iterations do not race with concurrent mutations.
1 participant
Summary
synchronizedblocks to preventConcurrentModificationExceptionHybridRnIapis deallocated during purchase/error listener callbacks for better diagnosticsChanges
Android (
HybridRnIap.kt)addPurchaseUpdatedListener,removePurchaseUpdatedListener,addPurchaseErrorListener,removePurchaseErrorListenernow usesynchronizedblockssendPurchaseUpdateandsendPurchaseErroriterate listeners withinsynchronizedblocksiOS (
HybridRnIap.swift)RnIapLog.warnmessages whenselfis deallocated (nil) in OpenIAP purchase and error listener callbacksHybridRnIaplifecycle issues (related to [iOS/Android] - High volume of purchases not being handled/notified #3150)Context
Investigating #3150 (purchases not being notified) revealed a potential thread safety issue in listener management on Android, and a diagnostics gap on iOS where purchase events could be silently dropped if the native module was deallocated.
Test plan
yarn typecheckpassesyarn lintpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores