Skip to content

Fix sync issues for dependency container access#402

Merged
ianrumac merged 1 commit intodevelopfrom
ir/fix/sync-anr
Apr 30, 2026
Merged

Fix sync issues for dependency container access#402
ianrumac merged 1 commit intodevelopfrom
ir/fix/sync-anr

Conversation

@ianrumac
Copy link
Copy Markdown
Collaborator

@ianrumac ianrumac commented Apr 30, 2026

Changes in this pull request

Checklist

  • All unit tests pass.
  • All UI tests pass.
  • Demo project builds and runs.
  • I added/updated tests or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have run ktlint in the main directory and fixed any issues.
  • I have updated the SDK documentation as well as the online docs.
  • I have reviewed the contributing guide

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 the dependencyContainer getter and replacing it with @Volatile on the backing field. The lock was unnecessary for the write-once field and created a deadlock cycle with the SynchronizedLazyImpl monitor on the entitlements lazy 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 synchronized getter 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: the subscriptionStatus lazy (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 subscriptionStatus lazy property.

Important Files Changed

Filename Overview
superwall/src/main/java/com/superwall/sdk/Superwall.kt Replaces synchronized(this) getter for _dependencyContainer with a @Volatile field — correct fix for the AB-BA deadlock; also includes minor ktlint-driven indentation reformatting.
superwall/src/test/java/com/superwall/sdk/SuperwallConfigureDeadlockTest.kt New regression guard that precisely arms the AB-BA deadlock interleaving; correctly detects if the lazy initializer re-enters the singleton monitor, though subscriptionStatus lazy is not also exercised.
CHANGELOG.md Adds 2.7.13 entry documenting the ANR/deadlock fix.
version.env Bumps version from 2.7.12 to 2.7.13.

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 complete
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
superwall/src/test/java/com/superwall/sdk/SuperwallConfigureDeadlockTest.kt:73-78
**`subscriptionStatus` lazy not covered by deadlock guard**

The `subscriptionStatus` property has the exact same AB-BA deadlock exposure as `entitlements` — its initializer is `by lazy { dependencyContainer.entitlements.status }`, which also calls through the `dependencyContainer` getter. The fix resolves both lazies simultaneously, but the regression guard only exercises `sw.entitlements`. A deadlock reintroduced specifically into the `subscriptionStatus` path (e.g., by wrapping its initializer in `synchronized`) would go undetected.

Consider adding a parallel read of `sw.subscriptionStatus` alongside `sw.entitlements` in Thread Y so both lazies are covered by the same test.

Reviews (1): Last reviewed commit: "Merge branch 'develop' into ir/fix/sync-..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@ianrumac ianrumac merged commit 296bcea into develop Apr 30, 2026
3 of 8 checks passed
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