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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions RevenueCat.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -3201,6 +3201,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

SWIFT_TREAT_WARNINGS_AS_ERRORS = YES;
VERSIONING_SYSTEM = "apple-generic";
VERSION_INFO_PREFIX = "";
Expand Down Expand Up @@ -3258,6 +3259,7 @@
SUPPORTS_MACCATALYST = YES;
SWIFT_COMPILATION_MODE = wholemodule;
SWIFT_OPTIMIZATION_LEVEL = "-O";
SWIFT_STRICT_CONCURRENCY = targeted;
SWIFT_TREAT_WARNINGS_AS_ERRORS = YES;
VALIDATE_PRODUCT = YES;
VERSIONING_SYSTEM = "apple-generic";
Expand Down
6 changes: 6 additions & 0 deletions Sources/Attribution/AttributionFetcher.swift
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,12 @@ class AttributionFetcher {

}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
extension AttributionFetcher: @unchecked Sendable {}

// MARK: - Private

private extension AttributionFetcher {

enum Error: Swift.Error {
Expand Down
4 changes: 3 additions & 1 deletion Sources/Attribution/AttributionPoster.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

import Foundation

class AttributionPoster {
final class AttributionPoster {

private let deviceCache: DeviceCache
private let currentUserProvider: CurrentUserProvider
Expand Down Expand Up @@ -234,3 +234,5 @@ class AttributionPoster {
}

}

extension AttributionPoster: Sendable {}
4 changes: 4 additions & 0 deletions Sources/Attribution/AttributionTypeFactory.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,7 @@ class AttributionTypeFactory {
}

}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
extension AttributionTypeFactory: @unchecked Sendable {}
4 changes: 4 additions & 0 deletions Sources/Caching/InMemoryCachedObject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,7 @@ class InMemoryCachedObject<T> {
return self.content.value.cachedObject
}
}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
extension InMemoryCachedObject: @unchecked Sendable {}
47 changes: 28 additions & 19 deletions Sources/Identity/CustomerInfoManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import Foundation

class CustomerInfoManager {

typealias CustomerInfoCompletion = @MainActor @Sendable (Result<CustomerInfo, BackendError>) -> Void

private(set) var lastSentCustomerInfo: CustomerInfo?
private let operationDispatcher: OperationDispatcher
private let deviceCache: DeviceCache
Expand All @@ -34,7 +36,7 @@ class CustomerInfoManager {

func fetchAndCacheCustomerInfo(appUserID: String,
isAppBackgrounded: Bool,
completion: ((Result<CustomerInfo, BackendError>) -> Void)?) {
completion: CustomerInfoCompletion?) {
self.backend.getCustomerInfo(appUserID: appUserID,
withRandomDelay: isAppBackgrounded) { result in
switch result {
Expand All @@ -48,7 +50,7 @@ class CustomerInfoManager {
}

if let completion = completion {
self.operationDispatcher.dispatchOnMainThread {
self.operationDispatcher.dispatchOnMainActor {
completion(result)
}
}
Expand All @@ -58,7 +60,7 @@ class CustomerInfoManager {

func fetchAndCacheCustomerInfoIfStale(appUserID: String,
isAppBackgrounded: Bool,
completion: ((Result<CustomerInfo, BackendError>) -> Void)?) {
completion: CustomerInfoCompletion?) {
let cachedCustomerInfo = self.cachedCustomerInfo(appUserID: appUserID)
let isCacheStale = self.deviceCache.isCustomerInfoCacheStale(appUserID: appUserID,
isAppBackgrounded: isAppBackgrounded)
Expand All @@ -74,7 +76,7 @@ class CustomerInfoManager {
}

if let completion = completion {
self.operationDispatcher.dispatchOnMainThread {
self.operationDispatcher.dispatchOnMainActor {
completion(.success(customerInfo))
}
}
Expand All @@ -91,13 +93,15 @@ class CustomerInfoManager {
func customerInfo(
appUserID: String,
fetchPolicy: CacheFetchPolicy,
completion: ((Result<CustomerInfo, BackendError>) -> Void)?
completion: CustomerInfoCompletion?
) {
switch fetchPolicy {
case .fromCacheOnly:
completion?(
Result(self.cachedCustomerInfo(appUserID: appUserID), .missingCachedCustomerInfo())
)
self.operationDispatcher.dispatchOnMainActor {
completion?(
Result(self.cachedCustomerInfo(appUserID: appUserID), .missingCachedCustomerInfo())
)
}

case .fetchCurrent:
self.systemInfo.isApplicationBackgrounded { isAppBackgrounded in
Expand All @@ -114,7 +118,7 @@ class CustomerInfoManager {
Logger.debug(Strings.customerInfo.vending_cache)
if let completion = completion {
completionCalled = true
self.operationDispatcher.dispatchOnMainThread {
self.operationDispatcher.dispatchOnMainActor {
completion(.success(infoFromCache))
}
}
Expand All @@ -139,7 +143,7 @@ class CustomerInfoManager {
if let infoFromCache = infoFromCache, !isCacheStale {
Logger.debug(Strings.customerInfo.vending_cache)
if let completion = completion {
self.operationDispatcher.dispatchOnMainThread {
self.operationDispatcher.dispatchOnMainActor {
completion(.success(infoFromCache))
}
}
Expand All @@ -153,7 +157,7 @@ class CustomerInfoManager {
}

func cachedCustomerInfo(appUserID: String) -> CustomerInfo? {
guard let customerInfoData = deviceCache.cachedCustomerInfoData(appUserID: appUserID) else {
guard let customerInfoData = self.deviceCache.cachedCustomerInfoData(appUserID: appUserID) else {
return nil
}

Expand All @@ -174,17 +178,17 @@ class CustomerInfoManager {
func cache(customerInfo: CustomerInfo, appUserID: String) {
do {
let jsonData = try JSONEncoder.default.encode(customerInfo)
deviceCache.cache(customerInfo: jsonData, appUserID: appUserID)
sendUpdateIfChanged(customerInfo: customerInfo)
self.deviceCache.cache(customerInfo: jsonData, appUserID: appUserID)
self.sendUpdateIfChanged(customerInfo: customerInfo)
} catch {
Logger.error(Strings.customerInfo.error_encoding_customerinfo(error))
}
}

func clearCustomerInfoCache(forAppUserID appUserID: String) {
customerInfoCacheLock.perform {
deviceCache.clearCustomerInfoCache(appUserID: appUserID)
lastSentCustomerInfo = nil
self.customerInfoCacheLock.perform {
self.deviceCache.clearCustomerInfoCache(appUserID: appUserID)
self.lastSentCustomerInfo = nil
}
}

Expand Down Expand Up @@ -226,9 +230,9 @@ class CustomerInfoManager {
}

private func sendUpdateIfChanged(customerInfo: CustomerInfo) {
customerInfoCacheLock.perform {
self.customerInfoCacheLock.perform {
guard !self.customerInfoObserversByIdentifier.isEmpty,
lastSentCustomerInfo != customerInfo else {
self.lastSentCustomerInfo != customerInfo else {
return
}

Expand Down Expand Up @@ -259,8 +263,13 @@ extension CustomerInfoManager {
return try await withCheckedThrowingContinuation { continuation in
return self.customerInfo(appUserID: appUserID,
fetchPolicy: fetchPolicy,
completion: continuation.resume)
completion: { @Sendable in continuation.resume(with: $0) })
}
}

}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
// - It has mutable state, but it's made thread-safe through `customerInfoCacheLock`.
extension CustomerInfoManager: @unchecked Sendable {}
12 changes: 9 additions & 3 deletions Sources/Identity/IdentityManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@

import Foundation

protocol CurrentUserProvider {
protocol CurrentUserProvider: Sendable {

var currentAppUserID: String { get }
var currentUserIsAnonymous: Bool { get }

}

protocol AttributeSyncing {
protocol AttributeSyncing: Sendable {

func syncSubscriberAttributes(currentAppUserID: String, completion: @escaping (() -> Void))
func syncSubscriberAttributes(currentAppUserID: String, completion: @escaping @Sendable () -> Void)
}

class IdentityManager: CurrentUserProvider {
Expand Down Expand Up @@ -142,6 +142,12 @@ private extension IdentityManager {
}
}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
extension IdentityManager: @unchecked Sendable {}

// MARK: - Private

private extension IdentityManager {

func resetUserIDCache() {
Expand Down
4 changes: 3 additions & 1 deletion Sources/Misc/DangerousSettings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Foundation
/**
Only use a Dangerous Setting if suggested by RevenueCat support team.
*/
@objc(RCDangerousSettings) public class DangerousSettings: NSObject {
@objc(RCDangerousSettings) public final class DangerousSettings: NSObject {

/**
* Disable or enable subscribing to the StoreKit queue. If this is disabled, RevenueCat won't observe
Expand Down Expand Up @@ -39,3 +39,5 @@ import Foundation
}

}

extension DangerousSettings: Sendable {}
4 changes: 4 additions & 0 deletions Sources/Misc/DateProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,7 @@ class DateProvider {
}

}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
extension DateProvider: @unchecked Sendable {}
2 changes: 1 addition & 1 deletion Sources/Misc/Deprecations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ extension CustomerInfo {
}

@available(*, deprecated, message: "Use NonSubscriptionTransaction")
private struct BackendParsedTransaction: StoreTransactionType {
private struct BackendParsedTransaction: StoreTransactionType, @unchecked Sendable {

let productIdentifier: String
let purchaseDate: Date
Expand Down
41 changes: 33 additions & 8 deletions Sources/Misc/OperationDispatcher.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,52 @@ import Foundation

class OperationDispatcher {

private let mainQueue = DispatchQueue.main
private let workerQueue = DispatchQueue(label: "OperationDispatcherWorkerQueue")
private let mainQueue: DispatchQueue = .main
private let workerQueue: DispatchQueue = .init(label: "OperationDispatcherWorkerQueue")
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.

👍

if Thread.isMainThread {
block()
} else {
mainQueue.async(execute: block)
self.mainQueue.async(execute: block)
}
}

func dispatchOnWorkerThread(withRandomDelay: Bool = false, block: @escaping () -> Void) {
func dispatchOnMainActor(_ block: @MainActor @escaping @Sendable () -> Void) {
Self.dispatchOnMainActor(block)
}

func dispatchOnWorkerThread(withRandomDelay: Bool = false, block: @escaping @Sendable () -> Void) {
if withRandomDelay {
let delay = Double.random(in: 0..<maxJitterInSeconds)
workerQueue.asyncAfter(deadline: .now() + delay, execute: block)
let delay = Double.random(in: 0..<self.maxJitterInSeconds)
self.workerQueue.asyncAfter(deadline: .now() + delay, execute: block)
} else {
self.workerQueue.async(execute: block)
}
}

}

extension OperationDispatcher {

static func dispatchOnMainActor(_ block: @MainActor @escaping @Sendable () -> Void) {
if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) {
Comment on lines +48 to +51
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

_ = Task {
await MainActor.run {
block()
}
}
} 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?

}
}

}

#if swift(<5.8)
// `DispatchQueue` is not `Sendable` as of Swift 5.7, but this class performs no mutations.
extension OperationDispatcher: @unchecked Sendable {}
#endif
7 changes: 6 additions & 1 deletion Sources/Misc/SandboxEnvironmentDetector.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import Foundation

/// A type that can determine if the current environment is sandbox.
protocol SandboxEnvironmentDetector {
protocol SandboxEnvironmentDetector: Sendable {

var isSandbox: Bool { get }

Expand Down Expand Up @@ -45,3 +45,8 @@ final class BundleSandboxEnvironmentDetector: SandboxEnvironmentDetector {
#endif

}

#if swift(<5.8)
// `Bundle` is not `Sendable` as of Swift 5.7, but this class performs no mutations.
extension BundleSandboxEnvironmentDetector: @unchecked Sendable {}
#endif
7 changes: 6 additions & 1 deletion Sources/Misc/SystemInfo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class SystemInfo {
}

static var platformHeader: String {
return Self.forceUniversalAppStore ? "iOS" : platformHeaderConstant
return Self.forceUniversalAppStore ? "iOS" : self.platformHeaderConstant
}

var identifierForVendor: String? {
Expand Down Expand Up @@ -145,6 +145,11 @@ class SystemInfo {

extension SystemInfo: SandboxEnvironmentDetector {}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
// - It includes `Bundle`, which isn't `Sendable` as of Swift 5.7.
extension SystemInfo: @unchecked Sendable {}

extension SystemInfo {

#if targetEnvironment(macCatalyst)
Expand Down
4 changes: 4 additions & 0 deletions Sources/Networking/Backend.swift
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ class Backend {

}

// @unchecked because:
// - Class is not `final` (it's mocked). This implicitly makes subclasses `Sendable` even if they're not thread-safe.
extension Backend: @unchecked Sendable {}

extension Backend {

enum QueueProvider {
Expand Down
4 changes: 4 additions & 0 deletions Sources/Networking/BackendConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,7 @@ extension BackendConfiguration {
}

}

// @unchecked because:
// - `OperationQueue` is not `Sendable` as of Swift 5.7
extension BackendConfiguration: @unchecked Sendable {}
Loading