Fix sync issues for dependency container access#402
Merged
Conversation
2ff9f82 to
c7650f0
Compare
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 fixes an AB-BA deadlock (tracked as SW-5092 / expo-superwall#194) that caused ANR in React Native by removing the
synchronized(this)block from thedependencyContainergetter and replacing it with@Volatileon the backing field. The lock was unnecessary for the write-once field and created a deadlock cycle with theSynchronizedLazyImplmonitor on theentitlementslazy property. A new regression test is included that arms the exact interleaving that previously deadlocked and asserts both threads complete within a tight timeout.Confidence Score: 4/5
Safe to merge — the fix is mechanically correct and well-tested; only a minor test-coverage gap remains.
The core change (replacing
synchronizedgetter with@Volatile) is the right tool for a write-once field and correctly breaks the lock cycle. The regression test is well-designed and will catch regressions. The only finding is a P2: thesubscriptionStatuslazy (same deadlock surface) is not covered by the test.superwall/src/test/java/com/superwall/sdk/SuperwallConfigureDeadlockTest.kt — consider extending the test to cover the
subscriptionStatuslazy property.Important Files Changed
synchronized(this)getter for_dependencyContainerwith a@Volatilefield — correct fix for the AB-BA deadlock; also includes minor ktlint-driven indentation reformatting.subscriptionStatuslazy is not also exercised.Sequence Diagram
sequenceDiagram participant X as worker-1 (setup) participant Y as worker-2 (lazy init) Note over X,Y: BEFORE fix — AB-BA deadlock X->>X: synchronized(Superwall) → Lock A acquired Y->>Y: SynchronizedLazyImpl.getValue → Lock B acquired X-->>Y: needs Lock B (entitlements lazy) Y-->>X: needs Lock A (old synchronized getter) Note over X,Y: ❌ Deadlock — both threads blocked Note over X,Y: AFTER fix X->>X: synchronized(Superwall) → Lock A acquired Y->>Y: SynchronizedLazyImpl.getValue → Lock B acquired Y->>Y: volatile read of _dependencyContainer (no lock) Y->>Y: lazy initialized → Lock B released X->>X: reads entitlements (lazy already warm) → Lock A released Note over X,Y: ✅ Both threads completePrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge branch 'develop' into ir/fix/sync-..." | Re-trigger Greptile