From d32483dcde5572d0a6a9d89d8b08ac0925852c36 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 12 May 2025 12:56:20 -0700 Subject: [PATCH 1/2] Model store minimize amount of locking needed Only lock when reading and writing from the dictionary, to minimize amount of locking when unnecessary --- .../OneSignalOSCore/Source/OSModelStore.swift | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSModelStore.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSModelStore.swift index 701d34ec2..1a2d6226e 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSModelStore.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSModelStore.swift @@ -105,14 +105,12 @@ open class OSModelStore: NSObject { // listen for changes to this model model.changeNotifier.subscribe(self) - - guard !hydrating else { - return - } - - self.changeSubscription.fire { modelStoreListener in - modelStoreListener.onAdded(model) - } + } + guard !hydrating else { + return + } + self.changeSubscription.fire { modelStoreListener in + modelStoreListener.onAdded(model) } } @@ -121,24 +119,28 @@ open class OSModelStore: NSObject { This can happen if remove email or SMS is called and it doesn't exist in the store. */ public func remove(_ id: String) { + var model: TModel? lock.withLock { OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSModelStore remove() called with model \(id)") - if let model = models[id] { + if let foundModel = models[id] { + model = foundModel models.removeValue(forKey: id) // persist the models (with removed model) to storage OneSignalUserDefaults.initShared().saveCodeableData(forKey: self.storeKey, withValue: self.models) - - // no longer listen for changes to this model - model.changeNotifier.unsubscribe(self) - - self.changeSubscription.fire { modelStoreListener in - modelStoreListener.onRemoved(model) - } } else { OneSignalLog.onesignalLog(.LL_ERROR, message: "OSModelStore cannot remove \(id) because it doesn't exist in the store.") + return } } + guard let model = model else { + return + } + // no longer listen for changes to this model + model.changeNotifier.unsubscribe(self) + self.changeSubscription.fire { modelStoreListener in + modelStoreListener.onRemoved(model) + } } /** @@ -167,13 +169,12 @@ extension OSModelStore: OSModelChangedHandler { // persist the changed models to storage lock.withLock { OneSignalUserDefaults.initShared().saveCodeableData(forKey: self.storeKey, withValue: self.models) - - guard !hydrating else { - return - } - self.changeSubscription.fire { modelStoreListener in - modelStoreListener.onUpdated(args) - } + } + guard !hydrating else { + return + } + self.changeSubscription.fire { modelStoreListener in + modelStoreListener.onUpdated(args) } } } From 96abd8b91da6128a7332a8029a7df2ca8bc8fb7d Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 12 May 2025 13:09:02 -0700 Subject: [PATCH 2/2] Make more no-ops if there is no user instance *Calling the `user` property of the `OneSignalUserManagerImpl` will return a mock user, the user instance, or call start(). * In some methods, if there is no user instance, don't trigger a start() call, to prevent deadlocks if these methods are called by start() itself. * Currently, only the properties model and subscription model may have update enqueued at the end of the start() method to enqueue any changes such as timezone and app version for the user instance, which exists by this point. * The other refactors are for safety. --- .../Source/Executors/OSUserExecutor.swift | 10 ++++++---- .../OSPropertiesModelStoreListener.swift | 9 +++++---- .../OSSubscriptionModelStoreListener.swift | 20 +++++++++++++++---- .../Source/OneSignalUserManagerImpl.swift | 6 +++--- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index 244c5ae04..0a8cc4f5b 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -119,8 +119,9 @@ class OSUserExecutor { // Translate the last request into a Create User request, if the current user is the same if let request = transferSubscriptionRequestQueue.last, + let userInstance = OneSignalUserManagerImpl.sharedInstance._user, OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.aliasId) { - createUser(OneSignalUserManagerImpl.sharedInstance.user) + createUser(userInstance) } } } @@ -396,9 +397,10 @@ extension OSUserExecutor { self.removeFromQueue(request) - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { + if let userInstance = OneSignalUserManagerImpl.sharedInstance._user, + OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { // Generate a Create User request, if it's still the current user - self.createUser(OneSignalUserManagerImpl.sharedInstance.user) + self.createUser(userInstance) } else { // This will hydrate the OneSignal ID for any pending requests self.createUser(aliasLabel: request.aliasLabel, aliasId: request.aliasId, identityModel: request.identityModelToUpdate) @@ -544,7 +546,7 @@ extension OSUserExecutor { } if let propertiesObject = parsePropertiesObjectResponse(response) { - OneSignalUserManagerImpl.sharedInstance.user.propertiesModel.hydrate(propertiesObject) + OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.hydrate(propertiesObject) } // Now parse email and sms subscriptions diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModelStoreListener.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModelStoreListener.swift index 611228801..d1cea8929 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModelStoreListener.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModelStoreListener.swift @@ -45,14 +45,15 @@ class OSPropertiesModelStoreListener: OSModelStoreListener { } func getUpdateModelDelta(_ args: OSModelChangedArgs) -> OSDelta? { - guard let _ = OSPropertiesSupportedProperty(rawValue: args.property) else { - OneSignalLog.onesignalLog(.LL_ERROR, message: "OSPropertiesModelStoreListener.getUpdateModelDelta encountered unsupported property: \(args.property)") + guard let _ = OSPropertiesSupportedProperty(rawValue: args.property), + let userInstance = OneSignalUserManagerImpl.sharedInstance._user + else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "OSPropertiesModelStoreListener.getUpdateModelDelta encountered unsupported property: \(args.property) or no user instance") return nil } - return OSDelta( name: OS_UPDATE_PROPERTIES_DELTA, - identityModelId: OneSignalUserManagerImpl.sharedInstance.user.identityModel.modelId, + identityModelId: userInstance.identityModel.modelId, model: args.model, property: args.property, value: args.newValue diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModelStoreListener.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModelStoreListener.swift index a58b1ac95..a51afc244 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModelStoreListener.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModelStoreListener.swift @@ -37,9 +37,13 @@ class OSSubscriptionModelStoreListener: OSModelStoreListener { } func getAddModelDelta(_ model: OSSubscriptionModel) -> OSDelta? { + guard let userInstance = OneSignalUserManagerImpl.sharedInstance._user else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "OSSubscriptionModelStoreListener.getAddModelDelta has no user instance") + return nil + } return OSDelta( name: OS_ADD_SUBSCRIPTION_DELTA, - identityModelId: OneSignalUserManagerImpl.sharedInstance.user.identityModel.modelId, + identityModelId: userInstance.identityModel.modelId, model: model, property: model.type.rawValue, // push, email, sms value: model.address ?? "" @@ -50,9 +54,13 @@ class OSSubscriptionModelStoreListener: OSModelStoreListener { The `property` and `value` is not needed for a remove operation, so just pass in some model data as placeholders. */ func getRemoveModelDelta(_ model: OSSubscriptionModel) -> OSDelta? { + guard let userInstance = OneSignalUserManagerImpl.sharedInstance._user else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "OSSubscriptionModelStoreListener.getRemoveModelDelta has no user instance") + return nil + } return OSDelta( name: OS_REMOVE_SUBSCRIPTION_DELTA, - identityModelId: OneSignalUserManagerImpl.sharedInstance.user.identityModel.modelId, + identityModelId: userInstance.identityModel.modelId, model: model, property: model.type.rawValue, // push, email, sms value: model.address ?? "" @@ -66,14 +74,18 @@ class OSSubscriptionModelStoreListener: OSModelStoreListener { // The user update call increases the session_count while the subscription update would update // something like the app_version. If the app_version hasn't changed since the last session, there // wouldn't be an update needed (among other system-level properties). - if let onesignalId = OneSignalUserManagerImpl.sharedInstance.user.identityModel.onesignalId { + guard let userInstance = OneSignalUserManagerImpl.sharedInstance._user else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "OSSubscriptionModelStoreListener.getUpdateModelDelta has no user instance") + return nil + } + if let onesignalId = userInstance.identityModel.onesignalId { let condition = OSIamFetchReadyCondition.sharedInstance(withId: onesignalId) condition.setSubscriptionUpdatePending(value: true) } return OSDelta( name: OS_UPDATE_SUBSCRIPTION_DELTA, - identityModelId: OneSignalUserManagerImpl.sharedInstance.user.identityModel.modelId, + identityModelId: userInstance.identityModel.modelId, model: args.model, property: args.property, value: args.newValue diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 1a11f4d5d..c662254bf 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -384,12 +384,12 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { } func isCurrentUser(_ externalId: String) -> Bool { - guard !externalId.isEmpty else { - OneSignalLog.onesignalLog(.LL_ERROR, message: "isCurrentUser called with empty externalId") + guard let userInstance = _user, !externalId.isEmpty else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "isCurrentUser called with empty externalId or no user instance") return false } - return user.identityModel.externalId == externalId + return userInstance.identityModel.externalId == externalId } /** Clears the existing user's data in preparation for hydration via a fetch user call.