Conversation
superwall/src/main/java/com/superwall/sdk/store/StoreManager.kt
Outdated
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes in this pull request
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.ktlintin the main directory and fixed any issues.Greptile Summary
This PR improves billing resilience and fixes several concurrency and lifecycle bugs. The paywall activity fix is correct and an improvement, but the
StoreManagerconcurrency change introduces a subtle but critical correctness regression.Key changes:
SuperwallPaywallActivity.kt– The scrim tap-outside handler in the bottom-sheet layout now callspaywallView()?.dismiss(PaywallResult.Declined(), ManualClose)instead of barefinish(), ensuring proper delegate callbacks and paywall result propagation on manual dismissal. This is a clean and correct fix.StoreManager.kt– ReplacesConcurrentHashMap.replace(id, state, newState)(an atomic CAS) withsynchronized(productsByFullId) { … }. This is problematic:productsByFullIdis aConcurrentHashMap, and thesynchronizedblock uses the object's intrinsic monitor lock, which is not held by any other accessor (cacheProduct,fetchNewProducts,getOrPut, etc.). As a result, the synchronization is one-sided and does not prevent concurrent modifications — e.g., a concurrentcacheProductcall can overwrite the map entry between the read and write inside thesynchronizedblock, potentially turning a freshly cachedLoadedstate back intoLoading. The original atomic CAS viareplace(key, expected, update)was the correct pattern forConcurrentHashMapand should be restored.Confidence Score: 2/5
synchronizedblock that is not honored by any other mutator in the class. This makes the "fix" a no-op in terms of thread safety while also introducing the possibility of overwriting aLoadedproduct state with a newLoadingstate, breaking downstream product fetching.StoreManager.kt— specifically the interaction between the newsynchronizedblock and the unsynchronized writes incacheProductandfetchNewProducts.Important Files Changed
ConcurrentHashMap.replace) with asynchronizedblock that is not used by any other accessor, making the "fix" ineffective and potentially introducing a new race condition where aLoadingstate can overwrite aLoadedstate.finish()scrim-tap handler in the bottom-sheet layout with a properpaywallView()?.dismiss(PaywallResult.Declined(), ManualClose)call, ensuring delegate callbacks are triggered on manual outside-tap dismissal.Sequence Diagram
sequenceDiagram participant CA as Coroutine A (fetchOrAwaitProducts) participant CB as Coroutine B (cacheProduct) participant Map as productsByFullId (ConcurrentHashMap) CA->>Map: getOrPut(id) → ProductState.Error Note over CA: enters synchronized(productsByFullId) CA->>Map: read: productsByFullId[id] is Error → true CB->>Map: productsByFullId[id] = Loaded(product) ← no lock held! CA->>Map: productsByFullId[id] = Loading(deferred) ← overwrites Loaded! Note over CA: newDeferreds[id] = deferred CA->>Map: (later) fetchNewProducts → triggers a new billing fetch for already-loaded product Note over Map: Product state corrupted: Loaded → Loading → re-fetched unnecessarilyLast reviewed commit: 8dfcbd4