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

Sendable support #1795

Merged
merged 6 commits into from
Aug 31, 2022
Merged

Sendable support #1795

merged 6 commits into from
Aug 31, 2022

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Aug 5, 2022

Fixes CSDK-379 and #1041 (comment)

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 classes, 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:

Depends on:

@NachoSoto NachoSoto added the feat A new feature label Aug 5, 2022
@@ -3168,6 +3168,7 @@
SDKROOT = iphoneos;
SUPPORTS_MACCATALYST = YES;
SWIFT_OPTIMIZATION_LEVEL = "-Onone";
SWIFT_STRICT_CONCURRENCY = targeted;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll see if we keep this or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not a bad idea to keep this so we ensure we maintain this level of correctness prior to Swift 6.0

@NachoSoto NachoSoto force-pushed the sendable branch 3 times, most recently from 0044e54 to 9bf8b8e Compare August 9, 2022 21:53
@NachoSoto NachoSoto force-pushed the sendable branch 2 times, most recently from abdf7a2 to 2aa6468 Compare August 9, 2022 23:09
@NachoSoto NachoSoto requested a review from a team August 9, 2022 23:15
@NachoSoto NachoSoto force-pushed the sendable branch 9 times, most recently from b5c1b42 to faed2ac Compare August 13, 2022 16:42
NachoSoto added a commit that referenced this pull request Aug 13, 2022
The type is now immutable, so the compiler ensures it's thread-safe.

For CSDK-379.
Extracted #1795 to make it easier to review this fix in isolation.
NachoSoto added a commit that referenced this pull request Aug 13, 2022
For [CSDK-379].
Extracted #1795 to make it easier to review this fix in isolation.

Also made `ASN1ObjectIdentifierBuilder` an `enum` since we don't need to create instances of it to use it.
@NachoSoto NachoSoto force-pushed the sendable branch 2 times, most recently from 7931d7e to 0e2538d Compare August 13, 2022 17:31
@NachoSoto
Copy link
Contributor Author

This is almost ready, but it will be a lot easier to review once all those dependent PRs are merged.

@NachoSoto NachoSoto force-pushed the sendable branch 2 times, most recently from 9bd795e to cc09992 Compare August 16, 2022 16:24
NachoSoto added a commit that referenced this pull request Aug 16, 2022
The type is now immutable, so the compiler ensures it's thread-safe.

For CSDK-379.
Extracted #1795 to make it easier to review this fix in isolation.
NachoSoto added a commit that referenced this pull request Aug 16, 2022
The type is now immutable, so the compiler ensures it's thread-safe.

For [CSDK-379].
Extracted from #1795 to make it easier to review this in isolation.
@NachoSoto NachoSoto force-pushed the sendable branch 3 times, most recently from 51387b7 to 5645f05 Compare August 16, 2022 17:33
NachoSoto added a commit that referenced this pull request Aug 22, 2022
The type is now immutable, so the compiler ensures it's thread-safe.

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

[CSDK-379]: https://revenuecats.atlassian.net/browse/CSDK-379?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
NachoSoto added a commit that referenced this pull request Aug 22, 2022
For [CSDK-379].
Extracted from #1795 to make it easier to review this in isolation.

Also made `ASN1ObjectIdentifierBuilder` an `enum` since we don't need to create instances of it to use it.

[CSDK-379]: https://revenuecats.atlassian.net/browse/CSDK-379?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
NachoSoto added a commit that referenced this pull request Aug 22, 2022
The type is now immutable, so the compiler ensures it's thread-safe.

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

[CSDK-379]: https://revenuecats.atlassian.net/browse/CSDK-379?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@NachoSoto
Copy link
Contributor Author

This is ready to review @RevenueCat/core-sdk

@NachoSoto
Copy link
Contributor Author

Bump @RevenueCat/core-sdk

Copy link
Contributor

@taquitos taquitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮‍💨
Nice work.

@NachoSoto
Copy link
Contributor Author

Merging this unless somebody else has any concerns or wants more time to look at it @RevenueCat/core-sdk

Comment on lines +48 to +51
extension OperationDispatcher {

static func dispatchOnMainActor(_ block: @MainActor @escaping @Sendable () -> Void) {
if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is cool

} else {
workerQueue.async(execute: block)
DispatchQueue.main.async(execute: block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be self.mainQueue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah good call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I know why I did this: there is some magic that allows passing a @MainActor block to only DispatchQueue.main, but not if we inject any random queue:

Screenshot 2022-08-31 at 13 03 10

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh right, that makes sense. Could you add a comment for the future?

private let maxJitterInSeconds: Double = 5

static let `default`: OperationDispatcher = .init()

func dispatchOnMainThread(_ block: @escaping () -> Void) {
func dispatchOnMainThread(_ block: @escaping @Sendable () -> Void) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be made private now? Is there any scenario where we should choose to go with main thread and not main actor even when Swift Concurrency is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we can get rid of this, but that seems like a bigger change. For existing code that does not use the new Swift "structured" concurrency, we might not want to change the semantics if we don't need to.

There's still a few places using dispatchOnMainThread that we might not want to change until we use @MainActor (or other custom actors) in more places.

Copy link
Contributor Author

@NachoSoto NachoSoto Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think, we can migrate more of these to dispatchOnMainActor in a following PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@NachoSoto NachoSoto merged commit 0bff3f6 into main Aug 31, 2022
@NachoSoto NachoSoto deleted the sendable branch August 31, 2022 20:30
NachoSoto pushed a commit that referenced this pull request Sep 8, 2022
**This is an automatic release.**

### Bugfixes
* `watchOS`: fixed crash when ran on single-target apps with Xcode 14 and before `watchOS 9.0` (#1895) via NachoSoto (@NachoSoto)
* `CustomerInfoManager`/`OfferingsManager`: improved display of underlying errors (#1888) via NachoSoto (@NachoSoto)
* `Offering`: improved confusing log for `PackageType.custom` (#1884) via NachoSoto (@NachoSoto)
* `PurchasesOrchestrator`: don't log warning if `allowSharingAppStoreAccount` setting was never explicitly set (#1885) via NachoSoto (@NachoSoto)
* Introduced type-safe `PurchasesError` and fixed some incorrect returned error types (#1879) via NachoSoto (@NachoSoto)
* `CustomerInfoManager`: fixed thread-unsafe implementation (#1878) via NachoSoto (@NachoSoto)
### New Features
* Disable SK1's `StoreKitWrapper` if SK2 is enabled and available (#1882) via NachoSoto (@NachoSoto)
* `Sendable` support (#1795) via NachoSoto (@NachoSoto)
### Other Changes
* Renamed `StoreKitWrapper` to `StoreKit1Wrapper` (#1886) via NachoSoto (@NachoSoto)
* Enabled `DEAD_CODE_STRIPPING` (#1887) via NachoSoto (@NachoSoto)
* `HTTPClient`: added `X-Client-Bundle-ID` and logged on SDK initialization (#1883) via NachoSoto (@NachoSoto)
* add link to SDK reference (#1872) via Andy Boedo (@aboedo)
* Added `StoreKit2Setting.shouldOnlyUseStoreKit2` (#1881) via NachoSoto (@NachoSoto)
* Introduced `TestLogHandler` to simplify how we test logged messages (#1858) via NachoSoto (@NachoSoto)
* `Integration Tests`: added test for purchasing `StoreProduct` instead of `Package` (#1875) via NachoSoto (@NachoSoto)
* `ErrorUtils`: added test to verify that returned errors can be converted to `ErrorCode` (#1871) via NachoSoto (@NachoSoto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants