From 35f6ed82541317993c2196d207198a96a33197be Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 18 Apr 2024 12:48:13 -0700 Subject: [PATCH 1/7] add test reproducing crash when encoding models with concurrency * Adds two tests that encode Identity Model and Properties Model while their aliases and tags dictionary are written to. * The tests try to add data, clear data, and encode these models concurrently --- .../OneSignalUserTests/.swiftlint.yml | 1 + .../OneSignalUserTests.swift | 44 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml b/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml index c6778fb1d..c22fc771a 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml @@ -1,3 +1,4 @@ # in tests, we may want to force cast and throw any errors disabled_rules: - force_cast + - variable_name diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index b87e3b126..d8111396c 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -93,4 +93,48 @@ final class OneSignalUserTests: XCTestCase { // 1. OpRepo: `deltaQueue.remove(at: index)` index out of bounds // 2. OSPropertyOperationExecutor: `deltaQueue.append(delta)` EXC_BAD_ACCESS } + + /** + This test reproduced a crash when the property model is being encoded. + */ + func testEncodingPropertiesModel_withConcurrency_doesNotCrash() throws { + /* Setup */ + let propertiesModel = OSPropertiesModel(changeNotifier: OSEventProducer()) + + /* When */ + DispatchQueue.concurrentPerform(iterations: 5_000) { i in + // 1. Add tags + for num in 0...9 { + propertiesModel.addTags(["\(i)tag\(num)": "value"]) + } + + // 2. Encode the model + OneSignalUserDefaults.initShared().saveCodeableData(forKey: "PropertyModel", withValue: propertiesModel) + + // 3. Clear the tags + propertiesModel.clearData() + } + } + + /** + This test reproduced a crash when the identity model is being encoded. + */ + func testEncodingIdentityModel_withConcurrency_doesNotCrash() throws { + /* Setup */ + let identityModel = OSIdentityModel(aliases: nil, changeNotifier: OSEventProducer()) + + /* When */ + DispatchQueue.concurrentPerform(iterations: 5_000) { i in + // 1. Add aliases + for num in 0...9 { + identityModel.addAliases(["\(i)alias\(num)": "value"]) + } + + // 2. Encode the model + OneSignalUserDefaults.initShared().saveCodeableData(forKey: "IdentityModel", withValue: identityModel) + + // 2. Clear the aliases + identityModel.clearData() + } + } } From 8cdf44629ac950baf3607a5ab7eb7f725e0533df Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 18 Apr 2024 12:41:27 -0700 Subject: [PATCH 2/7] lock access to model dictionaries during encode * Identity Model and Properties Model have access to their aliases and tags dictionary wrapped in a lock. However, this lock was missing from the encode method, and is necessary. --- .../OneSignalUser/Source/OSIdentityModel.swift | 6 ++++-- .../OneSignalUser/Source/OSPropertiesModel.swift | 10 ++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift index 72a5671e2..a70a13891 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift @@ -57,8 +57,10 @@ class OSIdentityModel: OSModel { } override func encode(with coder: NSCoder) { - super.encode(with: coder) - coder.encode(aliases, forKey: "aliases") + aliasesLock.locked { + super.encode(with: coder) + coder.encode(aliases, forKey: "aliases") + } } required init?(coder: NSCoder) { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift index edc23fc88..c04a559e3 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift @@ -102,10 +102,12 @@ class OSPropertiesModel: OSModel { } override func encode(with coder: NSCoder) { - super.encode(with: coder) - coder.encode(language, forKey: "language") - coder.encode(tags, forKey: "tags") - // ... and more + tagsLock.locked { + super.encode(with: coder) + coder.encode(language, forKey: "language") + coder.encode(tags, forKey: "tags") + // ... and more + } } required init?(coder: NSCoder) { From 8c28fba7680bce406ce948f2c41dd2256c35cd40 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 19 Apr 2024 10:55:14 -0700 Subject: [PATCH 3/7] No need to hydrate identity model twice * I noticed we were always hydrating the same identity model twice. * This is because we are parsing a fetch user response, we end up hydrating the identity model twice, unnecessarily, which drives caching it twice as well. * We already hydrate the identity model attached to the request. * Here, we were hydrating it again if the current user is the same. However, if the current user is the same, the model is the same and has already been hydrated. --- .../OneSignalUser/Source/Executors/OSUserExecutor.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index be0758ebc..0bb0ee47f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -520,10 +520,6 @@ extension OSUserExecutor { return } - if let identityObject = parseIdentityObjectResponse(response) { - OneSignalUserManagerImpl.sharedInstance.user.identityModel.hydrate(identityObject) - } - if let propertiesObject = parsePropertiesObjectResponse(response) { OneSignalUserManagerImpl.sharedInstance.user.propertiesModel.hydrate(propertiesObject) } From ff4705a42c9b62888aca94e1278f96c558a354c6 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 19 Apr 2024 11:34:53 -0700 Subject: [PATCH 4/7] Update Identity Model to cache itself less frequently * Removing multiple aliases via `removeAliases(array)` will now be done in one go: - Creates one OSDelta that will be parsed by the executor into individual Remove Alias Requests - Caches the new aliases once instead of in a loop for each label to remove * Hydrating aliases will now be done in one go as well: - Now, cache the new aliases once at the end. Prior, we cache the aliases after each alias as we hydrate. * In addition, put the call to `self.set` outside of the tagsLock, since the call to `self.set` will drive encoding itself, which also requires the tagsLock. The UnfairLock does not handle nested locking. --- .../OSIdentityOperationExecutor.swift | 2 +- .../Source/OSIdentityModel.swift | 69 +++++++++---------- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift index 06cfe3175..fd4674e9f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift @@ -124,7 +124,7 @@ class OSIdentityOperationExecutor: OSOperationExecutor { addRequestQueue.append(request) case OS_REMOVE_ALIAS_DELTA: - if let label = aliases.first?.key { + for (label, _) in aliases { let request = OSRequestRemoveAlias(labelToRemove: label, identityModel: model) removeRequestQueue.append(request) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift index a70a13891..d2826576e 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift @@ -31,17 +31,14 @@ import OneSignalOSCore class OSIdentityModel: OSModel { var onesignalId: String? { - aliasesLock.locked { - return aliases[OS_ONESIGNAL_ID] - } + return internalGetAlias(OS_ONESIGNAL_ID) } var externalId: String? { - aliasesLock.locked { - return aliases[OS_EXTERNAL_ID] - } + return internalGetAlias(OS_EXTERNAL_ID) } + // All access to aliases should go through helper methods with locking var aliases: [String: String] = [:] private let aliasesLock = UnfairLock() @@ -72,6 +69,24 @@ class OSIdentityModel: OSModel { self.aliases = aliases } + /** Threadsafe getter for an alias */ + private func internalGetAlias(_ label: String) -> String? { + aliasesLock.locked { + return self.aliases[label] + } + } + + /** Threadsafe setter or removal for aliases */ + private func internalAddAliases(_ aliases: [String: String]) { + aliasesLock.locked { + for (label, id) in aliases { + // Remove the alias if the ID field is "" + self.aliases[label] = id.isEmpty ? nil : id + } + } + self.set(property: "aliases", newValue: aliases) + } + /** Called to clear the model's data in preparation for hydration via a fetch user call. */ @@ -84,43 +99,27 @@ class OSIdentityModel: OSModel { // MARK: - Alias Methods func addAliases(_ aliases: [String: String]) { - aliasesLock.locked { - for (label, id) in aliases { - self.aliases[label] = id - } - self.set(property: "aliases", newValue: aliases) - } + internalAddAliases(aliases) } func removeAliases(_ labels: [String]) { - aliasesLock.locked { - for label in labels { - self.aliases.removeValue(forKey: label) - self.set(property: "aliases", newValue: [label: ""]) - } + let aliasesToRemoveAsDict = labels.reduce(into: [String: String]()) { result, label in + result[label] = "" } + internalAddAliases(aliasesToRemoveAsDict) } public override func hydrateModel(_ response: [String: Any]) { - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSIdentityModel hydrateModel()") - var newOnesignalId: String? - var newExternalId: String? - - aliasesLock.locked { - for property in response { - switch property.key { - case "external_id": - newExternalId = property.value as? String - aliases[OS_EXTERNAL_ID] = newExternalId - case "onesignal_id": - newOnesignalId = property.value as? String - aliases[OS_ONESIGNAL_ID] = newOnesignalId - default: - aliases[property.key] = property.value as? String - } - self.set(property: "aliases", newValue: aliases) - } + guard let remoteAliases = response as? [String: String] else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "OSIdentityModel.hydrateModel failed to parse response \(response) as Strings") + return } + + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSIdentityModel hydrateModel with aliases: \(remoteAliases)") + let newOnesignalId = remoteAliases[OS_ONESIGNAL_ID] + let newExternalId = remoteAliases[OS_EXTERNAL_ID] + + internalAddAliases(remoteAliases) fireUserStateChanged(newOnesignalId: newOnesignalId, newExternalId: newExternalId) } From ac60efc125bd532d22ba8c489991a4a0506b53c6 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 19 Apr 2024 15:25:58 -0700 Subject: [PATCH 5/7] Use a recursive lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Drop UnfairLock and use NSRecursiveLock * UnfairLock is fast, but it doesn’t support recursive locking. So, it crashes whenever you lock it twice from the same thread. * So far, we haven't encountered this, but this is possible, especially if we are not careful about how changes are propagated up. --- .../OneSignal.xcodeproj/project.pbxproj | 12 ----- .../Source/Utils/UnfairLock.swift | 47 ------------------- .../Source/OSIdentityModel.swift | 10 ++-- .../Source/OSPropertiesModel.swift | 12 ++--- 4 files changed, 11 insertions(+), 70 deletions(-) delete mode 100644 iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift diff --git a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj index 6cac7ee9d..434711331 100644 --- a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj +++ b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj @@ -53,7 +53,6 @@ 03E56DD328405F4A006AA1DA /* OneSignalAppDelegateOverrider.m in Sources */ = {isa = PBXBuildFile; fileRef = 03E56DD228405F4A006AA1DA /* OneSignalAppDelegateOverrider.m */; }; 16664C4C25DDB195003B8A14 /* NSTimeZoneOverrider.m in Sources */ = {isa = PBXBuildFile; fileRef = 16664C4B25DDB195003B8A14 /* NSTimeZoneOverrider.m */; }; 37E6B2BB19D9CAF300D0C601 /* UIKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 37E6B2BA19D9CAF300D0C601 /* UIKit.framework */; settings = {ATTRIBUTES = (Weak, ); }; }; - 3C05904B2B86BC9E00450D48 /* UnfairLock.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */; }; 3C0EF49E28A1DBCB00E5434B /* OSUserInternalImpl.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3C0EF49D28A1DBCB00E5434B /* OSUserInternalImpl.swift */; }; 3C115165289A259500565C41 /* OneSignalOSCore.docc in Sources */ = {isa = PBXBuildFile; fileRef = 3C115164289A259500565C41 /* OneSignalOSCore.docc */; }; 3C115171289A259500565C41 /* OneSignalOSCore.h in Headers */ = {isa = PBXBuildFile; fileRef = 3C115163289A259500565C41 /* OneSignalOSCore.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -944,7 +943,6 @@ 1AF75EAD1E8567FD0097B315 /* NSString+OneSignal.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSString+OneSignal.m"; sourceTree = ""; }; 37747F9319147D6500558FAD /* libOneSignal.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libOneSignal.a; sourceTree = BUILT_PRODUCTS_DIR; }; 37E6B2BA19D9CAF300D0C601 /* UIKit.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = UIKit.framework; path = System/Library/Frameworks/UIKit.framework; sourceTree = SDKROOT; }; - 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UnfairLock.swift; sourceTree = ""; }; 3C0EF49D28A1DBCB00E5434B /* OSUserInternalImpl.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OSUserInternalImpl.swift; sourceTree = ""; }; 3C115161289A259500565C41 /* OneSignalOSCore.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = OneSignalOSCore.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 3C115163289A259500565C41 /* OneSignalOSCore.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = OneSignalOSCore.h; sourceTree = ""; }; @@ -1640,14 +1638,6 @@ name = Frameworks; sourceTree = ""; }; - 3C0590492B86BC5300450D48 /* Utils */ = { - isa = PBXGroup; - children = ( - 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */, - ); - path = Utils; - sourceTree = ""; - }; 3C115162289A259500565C41 /* OneSignalOSCore */ = { isa = PBXGroup; children = ( @@ -1661,7 +1651,6 @@ isa = PBXGroup; children = ( 3C115163289A259500565C41 /* OneSignalOSCore.h */, - 3C0590492B86BC5300450D48 /* Utils */, 3C115188289ADEA300565C41 /* OSModelStore.swift */, 3C115186289ADE7700565C41 /* OSModelStoreListener.swift */, 3C115184289ADE4F00565C41 /* OSModel.swift */, @@ -3338,7 +3327,6 @@ 3C115187289ADE7700565C41 /* OSModelStoreListener.swift in Sources */, 3CE5F9E3289D88DC004A156E /* OSModelStoreChangedHandler.swift in Sources */, 3C2D8A5928B4C4E300BE41F6 /* OSDelta.swift in Sources */, - 3C05904B2B86BC9E00450D48 /* UnfairLock.swift in Sources */, 3C11518D289AF5E800565C41 /* OSModelChangedHandler.swift in Sources */, 3C8E6DF928A6D89E0031E48A /* OSOperationExecutor.swift in Sources */, ); diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift deleted file mode 100644 index 9d9f69ad2..000000000 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift +++ /dev/null @@ -1,47 +0,0 @@ -/* - Modified MIT License - - Copyright 2024 OneSignal - - Permission is hereby granted, free of charge, to any person obtaining a copy - of this software and associated documentation files (the "Software"), to deal - in the Software without restriction, including without limitation the rights - to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - copies of the Software, and to permit persons to whom the Software is - furnished to do so, subject to the following conditions: - - 1. The above copyright notice and this permission notice shall be included in - all copies or substantial portions of the Software. - - 2. All copies of substantial portions of the Software may only be used in connection - with services provided by OneSignal. - - THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - THE SOFTWARE. - */ - -// Taken from https://swiftrocks.com/thread-safety-in-swift -// Read http://www.russbishop.net/the-law for more information on why this is necessary -public final class UnfairLock { - private var _lock: UnsafeMutablePointer - - public init() { - _lock = UnsafeMutablePointer.allocate(capacity: 1) - _lock.initialize(to: os_unfair_lock()) - } - - deinit { - _lock.deallocate() - } - - public func locked(_ closure: () throws -> ReturnValue) rethrows -> ReturnValue { - os_unfair_lock_lock(_lock) - defer { os_unfair_lock_unlock(_lock) } - return try closure() - } -} diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift index d2826576e..194e6f16f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift @@ -40,7 +40,7 @@ class OSIdentityModel: OSModel { // All access to aliases should go through helper methods with locking var aliases: [String: String] = [:] - private let aliasesLock = UnfairLock() + private let aliasesLock = NSRecursiveLock() // TODO: We need to make this token secure public var jwtBearerToken: String? @@ -54,7 +54,7 @@ class OSIdentityModel: OSModel { } override func encode(with coder: NSCoder) { - aliasesLock.locked { + aliasesLock.withLock { super.encode(with: coder) coder.encode(aliases, forKey: "aliases") } @@ -71,14 +71,14 @@ class OSIdentityModel: OSModel { /** Threadsafe getter for an alias */ private func internalGetAlias(_ label: String) -> String? { - aliasesLock.locked { + aliasesLock.withLock { return self.aliases[label] } } /** Threadsafe setter or removal for aliases */ private func internalAddAliases(_ aliases: [String: String]) { - aliasesLock.locked { + aliasesLock.withLock { for (label, id) in aliases { // Remove the alias if the ID field is "" self.aliases[label] = id.isEmpty ? nil : id @@ -91,7 +91,7 @@ class OSIdentityModel: OSModel { Called to clear the model's data in preparation for hydration via a fetch user call. */ func clearData() { - aliasesLock.locked { + aliasesLock.withLock { self.aliases = [:] } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift index c04a559e3..3de274ec0 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift @@ -82,7 +82,7 @@ class OSPropertiesModel: OSModel { var timezoneId = TimeZone.current.identifier var tags: [String: String] = [:] - private let tagsLock = UnfairLock() + private let tagsLock = NSRecursiveLock() // MARK: - Initialization @@ -102,7 +102,7 @@ class OSPropertiesModel: OSModel { } override func encode(with coder: NSCoder) { - tagsLock.locked { + tagsLock.withLock { super.encode(with: coder) coder.encode(language, forKey: "language") coder.encode(tags, forKey: "tags") @@ -127,7 +127,7 @@ class OSPropertiesModel: OSModel { */ func clearData() { // TODO: What about language, lat, long? - tagsLock.locked { + tagsLock.withLock { self.tags = [:] } } @@ -135,7 +135,7 @@ class OSPropertiesModel: OSModel { // MARK: - Tag Methods func addTags(_ tags: [String: String]) { - tagsLock.locked { + tagsLock.withLock { for (key, value) in tags { self.tags[key] = value } @@ -144,7 +144,7 @@ class OSPropertiesModel: OSModel { } func removeTags(_ tags: [String]) { - tagsLock.locked { + tagsLock.withLock { var tagsToSend: [String: String] = [:] for tag in tags { self.tags.removeValue(forKey: tag) @@ -160,7 +160,7 @@ class OSPropertiesModel: OSModel { case "language": self.language = property.value as? String case "tags": - tagsLock.locked { + tagsLock.withLock { self.tags = property.value as? [String: String] ?? [:] } default: From db031ffc4d1c72b0a867269c6137c81da007406f Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 19 Apr 2024 15:36:23 -0700 Subject: [PATCH 6/7] Move properties model's change propagation outside of the lock * Keep the lock minimal, just around write and read access to the tags dictionary * Move the events-driving outside of the lock, that create Deltas and caches the model, etc. --- .../OneSignalUser/Source/OSPropertiesModel.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift index 3de274ec0..3a44f6e73 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift @@ -139,19 +139,19 @@ class OSPropertiesModel: OSModel { for (key, value) in tags { self.tags[key] = value } - self.set(property: "tags", newValue: tags) } + self.set(property: "tags", newValue: tags) } func removeTags(_ tags: [String]) { + var tagsToSend: [String: String] = [:] tagsLock.withLock { - var tagsToSend: [String: String] = [:] for tag in tags { self.tags.removeValue(forKey: tag) tagsToSend[tag] = "" } - self.set(property: "tags", newValue: tagsToSend) } + self.set(property: "tags", newValue: tagsToSend) } public override func hydrateModel(_ response: [String: Any]) { From 98908ee3c84db75fbe9ddd8de371f028840c9546 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 19 Apr 2024 15:33:04 -0700 Subject: [PATCH 7/7] [dev app] add remove tag * Adding tags with blank value will remove the tag --- iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m b/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m index 6833ff5dc..e4dbf20e9 100644 --- a/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m +++ b/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m @@ -121,10 +121,12 @@ - (IBAction)removeAliasButton:(UIButton *)sender { } - (IBAction)sendTagButton:(id)sender { - if (self.tagKey.text && self.tagKey.text.length - && self.tagValue.text && self.tagValue.text.length) { + if (self.tagValue.text && self.tagValue.text.length) { NSLog(@"Sending tag with key: %@ value: %@", self.tagKey.text, self.tagValue.text); [OneSignal.User addTagWithKey:self.tagKey.text value:self.tagValue.text]; + } else { + NSLog(@"Removing tag with key: %@", self.tagKey.text); + [OneSignal.User removeTag:self.tagKey.text]; } }