Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DeviceCache: Sendable conformance and fixed thread-safety #1823

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Aug 13, 2022

For CSDK-379.
Extracted #1795 to make it easier to review this fix in isolation.

handleUserDefaultsChanged isn't guaranteed to be called from any particular thread, so its access to appUserIDHasBeenSet wasn't thread-safe.

This is now guaranteed by Sendable.

In theory. In practice we are required to use @unchecked for now because of NotificationCenter and the class not being final. But without those I verified that the class does compile with Sendable, which means that there aren't any other potential issues.

@NachoSoto NachoSoto added the pr:fix A bug fix label Aug 13, 2022
@NachoSoto NachoSoto requested a review from a team August 13, 2022 17:04
@NachoSoto NachoSoto mentioned this pull request Aug 13, 2022
Base automatically changed from atomic-boolean to main August 15, 2022 18:04
For [CSDK-379].
Extracted #1795 to make it easier to review this fix in isolation.
@NachoSoto NachoSoto merged commit 9f76321 into main Aug 15, 2022
@NachoSoto NachoSoto deleted the device-cache-sendable branch August 15, 2022 18:04
@NachoSoto NachoSoto mentioned this pull request Aug 17, 2022
NachoSoto added a commit that referenced this pull request Aug 19, 2022
### Bugfixes
* `ErrorResponse`: don't add attribute errors to message if empty (#1844) via NachoSoto (@NachoSoto)
* Purchase cancellations: unify behavior between SK1 and SK2 (#1841) via NachoSoto (@NachoSoto)
* StoreKit 2: `PurchasesOrchestrator`: don't log "purchased product" if it was cancelled (#1840) via NachoSoto (@NachoSoto)
* `Backend`: fixed potential race conditions introduced by `OperationDispatcher.dispatchOnWorkerThread(withRandomDelay:)` (#1827) via NachoSoto (@NachoSoto)
* `DeviceCache`: `Sendable` conformance and fixed thread-safety (#1823) via NachoSoto (@NachoSoto)
* Directly send delegate customer info when delegate is set (always sends cached CustomerInfo value) (#1828) via Josh Holtz (@joshdholtz)
* `SystemInfo.finishTransactions`: made thread-safe (#1807) via NachoSoto (@NachoSoto)
* `Purchases.shared` and `Purchases.isConfigured` are now thread-safe (#1813) via NachoSoto (@NachoSoto)
* `PriceFormatterProvider: Sendable` conformance and fixed thread-safety (#1818) via NachoSoto (@NachoSoto)
* `StoreKitConfigTestCase.changeStorefront`: re-enabled on iOS 16 (#1811) via NachoSoto (@NachoSoto)

### Other Changes
* `DeviceCache`: no longer set cache timestamp before beginning request (#1839) via NachoSoto (@NachoSoto)
* `MagicWeatherSwiftUI`: updated to use `async` APIs (#1843) via NachoSoto (@NachoSoto)
* Release train (#1842) via Cesar de la Vega (@vegaro)
* Adds hotfixes section to RELEASING doc (#1837) via Cesar de la Vega (@vegaro)
* Update fastlane plugin (#1838) via Toni Rico (@tonidero)
* Update migration doc from didReceiveUpdatedCustomerInfo to receivedUpdatedCustomerInfo (#1836) via Josh Holtz (@joshdholtz)
* `PurchasesDelegate`: added test for latest cached customer info always being sent (#1830) via NachoSoto (@NachoSoto)
* `CallbackCache: Sendable` conformance (#1835) via NachoSoto (@NachoSoto)
* `CallbackCache`: simplified implementation using `Atomic` (#1834) via NachoSoto (@NachoSoto)
* `PurchasesLogInTests`: added test to verify `logIn` updates offerings cache (#1833) via NachoSoto (@NachoSoto)
* Created `PurchasesLoginTests` (#1832) via NachoSoto (@NachoSoto)
* `SwiftLint`: cleaned up output (#1821) via NachoSoto (@NachoSoto)
* Link to sdk reference (#1831) via aboedo (@aboedo)
* `Atomic: ExpressibleByBooleanLiteral` (#1822) via NachoSoto (@NachoSoto)
* `SwiftLint`: fixed build warning (#1820) via NachoSoto (@NachoSoto)
* Adds an approval job that will tag the release (#1815) via Cesar de la Vega (@vegaro)
* `Atomic: ExpressibleByNilLiteral` (#1804) via NachoSoto (@NachoSoto)
* `PurchasesAttributionDataTests`: fixed potential race condition in flaky test (#1805) via NachoSoto (@NachoSoto)
* Fixed warnings for unnecessary `try` (#1816) via NachoSoto (@NachoSoto)
* Moved `AttributionFetcherError` inside `AttributionFetcher` (#1808) via NachoSoto (@NachoSoto)
* Update documentation for presentCodeRedemptionSheet (#1817) via Joshua Liebowitz (@taquitos)
* `Dangerfile`: added "next_release" as supported label (#1810) via NachoSoto (@NachoSoto)
* PurchaseTester- Update Podfile.lock (#1814) via Joshua Liebowitz (@taquitos)
* Update to latest fastlane plugin (#1802) via Toni Rico (@tonidero)
* Clean up: moved `BackendIntegrationTests.xctestplan` to `TestPlans` folder (#1812) via NachoSoto (@NachoSoto)
* `SK2StoreProduct`: conditionally removed `@available` workaround (#1794) via NachoSoto (@NachoSoto)
* `SwiftLint`: fixed deprecation warning (#1809) via NachoSoto (@NachoSoto)
* Update gems (#1791) via Joshua Liebowitz (@taquitos)
* Replace usages of replace_in with replace_text_in_files action (#1803) via Toni Rico (@tonidero)
@revenuecat-ops revenuecat-ops mentioned this pull request Aug 24, 2022
NachoSoto added a commit that referenced this pull request Aug 31, 2022
Fixes [CSDK-379] and #1041 (comment)

[CSDK-379]: https://revenuecats.atlassian.net/browse/CSDK-379?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

### Main changes:
- `Sendable` conformances to all types that are thread-safe
- `@unchecked Sendable` for types that the compiler can't enforce, but with documentation as to why
- Made classes that aren't mocked `final` (note that this doesn't change the API, because none of these were `open` to begin with)
- Made some classes thread-safe that weren't (like `DeviceCache`)

For the non-`final` `class`es, I've actually managed to compile the SDK with all of them as `final` and `Sendable`, to make sure that making them `@unchecked Sendable` didn't hide any other issues.

### Future improvements:
- https://twitter.com/nachosoto/status/1557139992056500224?s=21&t=arWdvEzTIFANBvwqQ0vPiA
- https://twitter.com/nachosoto/status/1557141944777592832?s=21&t=arWdvEzTIFANBvwqQ0vPiA
- https://twitter.com/nachosoto/status/1557143374922059776?s=21&t=arWdvEzTIFANBvwqQ0vPiA

### Depends on:
- #1794
- #1804
- #1806
- #1807
- #1808
- #1813
- #1822
- #1823
- #1824
- #1825
- #1826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants