From a554e302315ca357ebdfaff7f213843e162a7956 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 22 Feb 2024 10:52:51 -0800 Subject: [PATCH 1/8] Add a unit test to reproduce OpRepo crash --- .../OneSignalUserTests.swift | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index a53197056..ade8d4f19 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -29,6 +29,7 @@ import XCTest import OneSignalCore import OneSignalCoreMocks import OneSignalUserMocks +import OneSignalOSCore @testable import OneSignalUser final class OneSignalUserTests: XCTestCase { @@ -62,4 +63,34 @@ final class OneSignalUserTests: XCTestCase { XCTAssertEqual(identityModelStoreExternalId, "my-external-id") XCTAssertEqual(userInstanceExternalId, "my-external-id") } + + /** + This test reproduces a crash in the Operation Repo's flushing delta queue. + It is possible for two threads to flush concurrently. + However, this test does not crash 100% of the time. + */ + func testOperationRepoFlushingConcurrency() throws { + /* Setup */ + OneSignalCore.setSharedClient(MockOneSignalClient()) + + /* When */ + + // 1. Enqueue 10 Deltas to the Operation Repo + for num in 0...9 { + OneSignalUserManagerImpl.sharedInstance.addTag(key: "tag\(num)", value: "value") + } + + // 2. Flush the delta queue from 4 multiple threads + for _ in 1...4 { + DispatchQueue.global().async { + print("🧪 flushDeltaQueue on thread \(Thread.current)") + OSOperationRepo.sharedInstance.flushDeltaQueue() + } + } + + /* Then */ + // There are two places that can crash, as multiple threads are manipulating arrays: + // 1. OpRepo: `deltaQueue.remove(at: index)` index out of bounds + // 2. OSPropertyOperationExecutor: `deltaQueue.append(delta)` EXC_BAD_ACCESS + } } From d42c494e921214b1a3f2e4db6318477dbcae3444 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 22 Feb 2024 10:52:25 -0800 Subject: [PATCH 2/8] Add a Lock class --- .../OneSignal.xcodeproj/project.pbxproj | 12 +++++ .../Source/Utils/UnfairLock.swift | 47 +++++++++++++++++++ 2 files changed, 59 insertions(+) create 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 434711331..6cac7ee9d 100644 --- a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj +++ b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj @@ -53,6 +53,7 @@ 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, ); }; }; @@ -943,6 +944,7 @@ 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 = ""; }; @@ -1638,6 +1640,14 @@ name = Frameworks; sourceTree = ""; }; + 3C0590492B86BC5300450D48 /* Utils */ = { + isa = PBXGroup; + children = ( + 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */, + ); + path = Utils; + sourceTree = ""; + }; 3C115162289A259500565C41 /* OneSignalOSCore */ = { isa = PBXGroup; children = ( @@ -1651,6 +1661,7 @@ isa = PBXGroup; children = ( 3C115163289A259500565C41 /* OneSignalOSCore.h */, + 3C0590492B86BC5300450D48 /* Utils */, 3C115188289ADEA300565C41 /* OSModelStore.swift */, 3C115186289ADE7700565C41 /* OSModelStoreListener.swift */, 3C115184289ADE4F00565C41 /* OSModel.swift */, @@ -3327,6 +3338,7 @@ 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 new file mode 100644 index 000000000..9d9f69ad2 --- /dev/null +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift @@ -0,0 +1,47 @@ +/* + 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() + } +} From 96b1ad757fca5c267dd1a9f8834e8e8c6334b6fe Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 22 Feb 2024 10:54:01 -0800 Subject: [PATCH 3/8] Add lock around accessing OpRepo delta queue * Add lock around appending to the queue * Add lock around entire logic of flushing the delta queue, including enqueuing and flushing in the executors --- .../Source/OSOperationRepo.swift | 59 ++++++++++--------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift index d13d38162..c64ddf987 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift @@ -35,11 +35,12 @@ import OneSignalCore public class OSOperationRepo: NSObject { public static let sharedInstance = OSOperationRepo() private var hasCalledStart = false + private let deltaQueueLock = UnfairLock() // Maps delta names to the interfaces for the operation executors var deltasToExecutorMap: [String: OSOperationExecutor] = [:] var executors: [OSOperationExecutor] = [] - var deltaQueue: [OSDelta] = [] + private var deltaQueue: [OSDelta] = [] // TODO: This could come from a config, plist, method, remote params var pollIntervalMilliseconds = Int(POLL_INTERVAL_MS) @@ -102,10 +103,12 @@ public class OSOperationRepo: NSObject { } start() OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo enqueueDelta: \(delta)") - deltaQueue.append(delta) - // Persist the deltas (including new delta) to storage - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) + deltaQueueLock.locked { + deltaQueue.append(delta) + // Persist the deltas (including new delta) to storage + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) + } } @objc public func flushDeltaQueue(inBackground: Bool = false) { @@ -123,35 +126,37 @@ public class OSOperationRepo: NSObject { } start() - if !deltaQueue.isEmpty { - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo flushDeltaQueue in background: \(inBackground) with queue: \(deltaQueue)") - } - var index = 0 - for delta in deltaQueue { - if let executor = deltasToExecutorMap[delta.name] { - executor.enqueueDelta(delta) - deltaQueue.remove(at: index) - } else { - // keep in queue if no executor matches, we may not have the executor available yet - index += 1 + deltaQueueLock.locked { + if !deltaQueue.isEmpty { + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo flushDeltaQueue in background: \(inBackground) with queue: \(deltaQueue)") } - } - // Persist the deltas (including removed deltas) to storage after they are divvy'd up to executors. - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) + var index = 0 + for delta in deltaQueue { + if let executor = deltasToExecutorMap[delta.name] { + executor.enqueueDelta(delta) + deltaQueue.remove(at: index) + } else { + // keep in queue if no executor matches, we may not have the executor available yet + index += 1 + } + } - for executor in executors { - executor.cacheDeltaQueue() - } + // Persist the deltas (including removed deltas) to storage after they are divvy'd up to executors. + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) - for executor in executors { - executor.processDeltaQueue(inBackground: inBackground) - } + for executor in executors { + executor.cacheDeltaQueue() + } - if inBackground { - OSBackgroundTaskManager.endBackgroundTask(OPERATION_REPO_BACKGROUND_TASK) - } + for executor in executors { + executor.processDeltaQueue(inBackground: inBackground) + } + if inBackground { + OSBackgroundTaskManager.endBackgroundTask(OPERATION_REPO_BACKGROUND_TASK) + } + } } } From 93435a62e3bacd6f991e63049929b9dabddd5e3b Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 29 Feb 2024 12:03:04 -0800 Subject: [PATCH 4/8] Synchronize the background tasks dictionary There was a report of a crash related to this dictionary and while I have not been able to reproduce by hammering many threads to call methods manipulating the tasks dictionary, access to them should be synchronized. --- .../Source/OSBackgroundTaskHandlerImpl.m | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/Source/OSBackgroundTaskHandlerImpl.m b/iOS_SDK/OneSignalSDK/Source/OSBackgroundTaskHandlerImpl.m index 79b2dac43..9db3fdcf3 100644 --- a/iOS_SDK/OneSignalSDK/Source/OSBackgroundTaskHandlerImpl.m +++ b/iOS_SDK/OneSignalSDK/Source/OSBackgroundTaskHandlerImpl.m @@ -50,21 +50,30 @@ - (void)beginBackgroundTask:(NSString * _Nonnull)taskIdentifier { @"OSBackgroundTaskManagerImpl: expirationHandler called for %@", taskIdentifier]]; [self endBackgroundTask:taskIdentifier]; }]; - tasks[taskIdentifier] = [NSNumber numberWithUnsignedLong:uiIdentifier]; + @synchronized (tasks) { + tasks[taskIdentifier] = [NSNumber numberWithUnsignedLong:uiIdentifier]; + } } - (void)endBackgroundTask:(NSString * _Nonnull)taskIdentifier { - UIBackgroundTaskIdentifier uiIdentifier = [[tasks objectForKey:taskIdentifier] unsignedLongValue]; - [OneSignalLog onesignalLog:ONE_S_LL_DEBUG - message:[NSString stringWithFormat: - @"OSBackgroundTaskManagerImpl:endBackgroundTask: %@ with UIBackgroundTaskIdentifier %lu", - taskIdentifier, (unsigned long)uiIdentifier]]; - [UIApplication.sharedApplication endBackgroundTask:uiIdentifier]; + @synchronized (tasks) { + UIBackgroundTaskIdentifier uiIdentifier = [[tasks objectForKey:taskIdentifier] unsignedLongValue]; + [OneSignalLog onesignalLog:ONE_S_LL_DEBUG + message:[NSString stringWithFormat: + @"OSBackgroundTaskManagerImpl:endBackgroundTask: %@ with UIBackgroundTaskIdentifier %lu", + taskIdentifier, (unsigned long)uiIdentifier]]; + [UIApplication.sharedApplication endBackgroundTask:uiIdentifier]; + } [self setTaskInvalid:taskIdentifier]; } +/** + This method is called when the background task ends or directly by other services within the SDK + */ - (void)setTaskInvalid:(NSString * _Nonnull)taskIdentifier { - tasks[taskIdentifier] = [NSNumber numberWithUnsignedLong:UIBackgroundTaskInvalid]; + @synchronized (tasks) { + tasks[taskIdentifier] = [NSNumber numberWithUnsignedLong:UIBackgroundTaskInvalid]; + } } @end From a21404f30b0dc7c88ca08d29c9b8664c9761a824 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 29 Feb 2024 12:56:30 -0800 Subject: [PATCH 5/8] Properties model: add locking to tags dictionary I was able to create crashes by hammering threads to add tags concurrently. Adding the lock prevented crashes, and behavior seems as expected. Unfortunately, the crashes were very flaky when trying to reproduce in tests, they would crash in much less than 50% of runs. Producing the crashes in the example app was much more consistent. Hence why there is no unit test to reproduce this. --- .../Source/OSPropertiesModel.swift | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift index eb9e59bc1..edc23fc88 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift @@ -82,6 +82,7 @@ class OSPropertiesModel: OSModel { var timezoneId = TimeZone.current.identifier var tags: [String: String] = [:] + private let tagsLock = UnfairLock() // MARK: - Initialization @@ -124,25 +125,31 @@ class OSPropertiesModel: OSModel { */ func clearData() { // TODO: What about language, lat, long? - self.tags = [:] + tagsLock.locked { + self.tags = [:] + } } // MARK: - Tag Methods func addTags(_ tags: [String: String]) { - for (key, value) in tags { - self.tags[key] = value + tagsLock.locked { + 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] = [:] - for tag in tags { - self.tags.removeValue(forKey: tag) - tagsToSend[tag] = "" + tagsLock.locked { + 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]) { @@ -151,7 +158,9 @@ class OSPropertiesModel: OSModel { case "language": self.language = property.value as? String case "tags": - self.tags = property.value as? [String: String] ?? [:] + tagsLock.locked { + self.tags = property.value as? [String: String] ?? [:] + } default: OneSignalLog.onesignalLog(.LL_DEBUG, message: "Not hydrating properties model for property: \(property)") } From 31834636d836f37dc1d0f96d250c4311defe6f04 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 29 Feb 2024 14:12:58 -0800 Subject: [PATCH 6/8] Identity model: add locking to aliases dictionary I was able to create crashes by hammering threads to add aliases and get aliases concurrently. Adding the lock prevented crashes, and behavior seems as expected. I also had to lock the getters for onesignalId and externalId Unfortunately, the crashes were very flaky when trying to reproduce in tests, they would crash in much less than 50% of runs. Producing the crashes in the example app was much more consistent. Hence why there is no unit test to reproduce this. --- .../Source/OSIdentityModel.swift | 53 ++++++++++++------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift index b915030cc..72a5671e2 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift @@ -31,14 +31,19 @@ import OneSignalOSCore class OSIdentityModel: OSModel { var onesignalId: String? { - return aliases[OS_ONESIGNAL_ID] + aliasesLock.locked { + return aliases[OS_ONESIGNAL_ID] + } } var externalId: String? { - return aliases[OS_EXTERNAL_ID] + aliasesLock.locked { + return aliases[OS_EXTERNAL_ID] + } } var aliases: [String: String] = [:] + private let aliasesLock = UnfairLock() // TODO: We need to make this token secure public var jwtBearerToken: String? @@ -69,22 +74,28 @@ class OSIdentityModel: OSModel { Called to clear the model's data in preparation for hydration via a fetch user call. */ func clearData() { - self.aliases = [:] + aliasesLock.locked { + self.aliases = [:] + } } // MARK: - Alias Methods func addAliases(_ aliases: [String: String]) { - for (label, id) in aliases { - self.aliases[label] = id + aliasesLock.locked { + for (label, id) in aliases { + self.aliases[label] = id + } + self.set(property: "aliases", newValue: aliases) } - self.set(property: "aliases", newValue: aliases) } func removeAliases(_ labels: [String]) { - for label in labels { - self.aliases.removeValue(forKey: label) - self.set(property: "aliases", newValue: [label: ""]) + aliasesLock.locked { + for label in labels { + self.aliases.removeValue(forKey: label) + self.set(property: "aliases", newValue: [label: ""]) + } } } @@ -93,18 +104,20 @@ class OSIdentityModel: OSModel { var newOnesignalId: String? var newExternalId: String? - 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 + 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) } - self.set(property: "aliases", newValue: aliases) } fireUserStateChanged(newOnesignalId: newOnesignalId, newExternalId: newExternalId) } From cb88aa60befc548204020addc07d9b038444d80d Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 4 Mar 2024 00:30:20 -0800 Subject: [PATCH 7/8] Property Executor: Add private dispatch queue * Reproduced crashes by creating multiple async threads that added and removed tags concurrently. * Added a private dispatch queue to synchronize access to the delta queue and request queue. * Crashes no longer happened after this change. * It is possible for the executor to be flushing while a client response is received and modify the request queue. * Additionally, there are some code paths that enqueue and update request but does not go through the operation repo, such as updating session count at the start of a new session. --- .../OSPropertyOperationExecutor.swift | 120 ++++++++++-------- 1 file changed, 68 insertions(+), 52 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift index f01f60b89..7474284a7 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift @@ -33,6 +33,9 @@ class OSPropertyOperationExecutor: OSOperationExecutor { var deltaQueue: [OSDelta] = [] var updateRequestQueue: [OSRequestUpdateProperties] = [] + // The property executor dispatch queue, serial. This synchronizes access to `deltaQueue` and `updateRequestQueue`. + private let dispatchQueue = DispatchQueue(label: "OneSignal.OSPropertyOperationExecutor", target: .global()) + init() { // Read unfinished deltas from cache, if any... // Note that we should only have deltas for the current user as old ones are flushed.. @@ -78,42 +81,49 @@ class OSPropertyOperationExecutor: OSOperationExecutor { } func enqueueDelta(_ delta: OSDelta) { - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSPropertyOperationExecutor enqueue delta\(delta)") - deltaQueue.append(delta) + self.dispatchQueue.async { + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSPropertyOperationExecutor enqueue delta\(delta)") + self.deltaQueue.append(delta) + } } func cacheDeltaQueue() { - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_DELTA_QUEUE_KEY, withValue: self.deltaQueue) + self.dispatchQueue.async { + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_DELTA_QUEUE_KEY, withValue: self.deltaQueue) + } } func processDeltaQueue(inBackground: Bool) { - if !deltaQueue.isEmpty { - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSPropertyOperationExecutor processDeltaQueue with queue: \(deltaQueue)") - } - for delta in deltaQueue { - guard let model = delta.model as? OSPropertiesModel else { - // Log error - continue + self.dispatchQueue.async { + if !self.deltaQueue.isEmpty { + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSPropertyOperationExecutor processDeltaQueue with queue: \(self.deltaQueue)") } + for delta in self.deltaQueue { + guard let model = delta.model as? OSPropertiesModel else { + // Log error + continue + } - let request = OSRequestUpdateProperties( - properties: [delta.property: delta.value], - deltas: nil, - refreshDeviceMetadata: false, // Sort this out. - modelToUpdate: model, - identityModel: OneSignalUserManagerImpl.sharedInstance.user.identityModel // TODO: Make sure this is ok - ) - updateRequestQueue.append(request) - } - self.deltaQueue = [] // TODO: Check that we can simply clear all the deltas in the deltaQueue + let request = OSRequestUpdateProperties( + properties: [delta.property: delta.value], + deltas: nil, + refreshDeviceMetadata: false, // Sort this out. + modelToUpdate: model, + identityModel: OneSignalUserManagerImpl.sharedInstance.user.identityModel // TODO: Make sure this is ok + ) + self.updateRequestQueue.append(request) + } + self.deltaQueue = [] // TODO: Check that we can simply clear all the deltas in the deltaQueue - // persist executor's requests (including new request) to storage - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) + // persist executor's requests (including new request) to storage + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_DELTA_QUEUE_KEY, withValue: self.deltaQueue) // This should be empty, can remove instead? - processRequestQueue(inBackground: inBackground) + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_DELTA_QUEUE_KEY, withValue: self.deltaQueue) // This should be empty, can remove instead? + self.processRequestQueue(inBackground: inBackground) + } } + // This method is called by `processDeltaQueue` only and does not need to be added to the dispatchQueue. func processRequestQueue(inBackground: Bool) { if updateRequestQueue.isEmpty { return @@ -141,38 +151,42 @@ class OSPropertyOperationExecutor: OSOperationExecutor { OneSignalCore.sharedClient().execute(request) { _ in // On success, remove request from cache, and we do need to hydrate // TODO: We need to hydrate after all ? What why ? - self.updateRequestQueue.removeAll(where: { $0 == request}) - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) - if inBackground { - OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + self.dispatchQueue.async { + self.updateRequestQueue.removeAll(where: { $0 == request}) + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) + if inBackground { + OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + } } } onFailure: { error in OneSignalLog.onesignalLog(.LL_ERROR, message: "OSPropertyOperationExecutor update properties request failed with error: \(error.debugDescription)") - if let nsError = error as? NSError { - let responseType = OSNetworkingUtils.getResponseStatusType(nsError.code) - if responseType == .missing { - // remove from cache and queue - self.updateRequestQueue.removeAll(where: { $0 == request}) - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) - else { - if inBackground { - OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + self.dispatchQueue.async { + if let nsError = error as? NSError { + let responseType = OSNetworkingUtils.getResponseStatusType(nsError.code) + if responseType == .missing { + // remove from cache and queue + self.updateRequestQueue.removeAll(where: { $0 == request}) + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) + // Logout if the user in the SDK is the same + guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + else { + if inBackground { + OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + } + return } - return + // The subscription has been deleted along with the user, so remove the subscription_id but keep the same push subscription model + OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.subscriptionId = nil + OneSignalUserManagerImpl.sharedInstance._logout() + } else if responseType != .retryable { + // Fail, no retry, remove from cache and queue + self.updateRequestQueue.removeAll(where: { $0 == request}) + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) } - // The subscription has been deleted along with the user, so remove the subscription_id but keep the same push subscription model - OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.subscriptionId = nil - OneSignalUserManagerImpl.sharedInstance._logout() - } else if responseType != .retryable { - // Fail, no retry, remove from cache and queue - self.updateRequestQueue.removeAll(where: { $0 == request}) - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) } - } - if inBackground { - OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + if inBackground { + OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + } } } } @@ -201,8 +215,10 @@ extension OSPropertyOperationExecutor { } } } else { - updateRequestQueue.append(request) - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) + self.dispatchQueue.async { + self.updateRequestQueue.append(request) + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) + } } } } From c45c08157bbebf0efcf24c6662012929779fabdd Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 4 Mar 2024 10:16:20 -0800 Subject: [PATCH 8/8] OpRepo: Use private dispatch queue instead of lock * In a previous commit, an unfair lock was used to synchronize access to the delta queue and synchronize flushing behavior * A dispatch queue seems more appropriate for the Operation Repo to use considering it already polls and flushes on a global queue. * Without the lock or dispatch queue, I reproduced crashes by creating multiple async threads that either added tags or called to flush the operation repo. * With the dispatch queue, those crashes do no happen and behavior seems as expected. --- .../Source/OSOperationRepo.swift | 71 ++++++++++--------- .../Source/OneSignalUserManagerImpl.swift | 2 +- .../OneSignalUserTests.swift | 2 +- 3 files changed, 40 insertions(+), 35 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift index c64ddf987..6da7b4553 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/OSOperationRepo.swift @@ -35,7 +35,9 @@ import OneSignalCore public class OSOperationRepo: NSObject { public static let sharedInstance = OSOperationRepo() private var hasCalledStart = false - private let deltaQueueLock = UnfairLock() + + // The Operation Repo dispatch queue, serial. This synchronizes access to `deltaQueue` and flushing behavior. + private let dispatchQueue = DispatchQueue(label: "OneSignal.OSOperationRepo", target: .global()) // Maps delta names to the interfaces for the operation executors var deltasToExecutorMap: [String: OSOperationExecutor] = [:] @@ -63,7 +65,7 @@ public class OSOperationRepo: NSObject { OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo calling start()") // register as user observer NotificationCenter.default.addObserver(self, - selector: #selector(self.flushDeltaQueue), + selector: #selector(self.addFlushDeltaQueueToDispatchQueue), name: Notification.Name(OS_ON_USER_WILL_CHANGE), object: nil) // Read the Deltas from cache, if any... @@ -77,7 +79,7 @@ public class OSOperationRepo: NSObject { } private func pollFlushQueue() { - DispatchQueue.global().asyncAfter(deadline: .now() + .milliseconds(pollIntervalMilliseconds)) { [weak self] in + self.dispatchQueue.asyncAfter(deadline: .now() + .milliseconds(pollIntervalMilliseconds)) { [weak self] in self?.flushDeltaQueue() self?.pollFlushQueue() } @@ -102,16 +104,21 @@ public class OSOperationRepo: NSObject { return } start() - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo enqueueDelta: \(delta)") - - deltaQueueLock.locked { - deltaQueue.append(delta) + self.dispatchQueue.async { + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo enqueueDelta: \(delta)") + self.deltaQueue.append(delta) // Persist the deltas (including new delta) to storage OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) } } - @objc public func flushDeltaQueue(inBackground: Bool = false) { + @objc public func addFlushDeltaQueueToDispatchQueue(inBackground: Bool = false) { + self.dispatchQueue.async { + self.flushDeltaQueue(inBackground: inBackground) + } + } + + private func flushDeltaQueue(inBackground: Bool = false) { guard !paused else { OneSignalLog.onesignalLog(.LL_DEBUG, message: "OSOperationRepo not flushing queue due to being paused") return @@ -125,38 +132,36 @@ public class OSOperationRepo: NSObject { OSBackgroundTaskManager.beginBackgroundTask(OPERATION_REPO_BACKGROUND_TASK) } - start() + self.start() - deltaQueueLock.locked { - if !deltaQueue.isEmpty { - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo flushDeltaQueue in background: \(inBackground) with queue: \(deltaQueue)") - } + if !self.deltaQueue.isEmpty { + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSOperationRepo flushDeltaQueue in background: \(inBackground) with queue: \(self.deltaQueue)") + } - var index = 0 - for delta in deltaQueue { - if let executor = deltasToExecutorMap[delta.name] { - executor.enqueueDelta(delta) - deltaQueue.remove(at: index) - } else { - // keep in queue if no executor matches, we may not have the executor available yet - index += 1 - } + var index = 0 + for delta in self.deltaQueue { + if let executor = self.deltasToExecutorMap[delta.name] { + executor.enqueueDelta(delta) + self.deltaQueue.remove(at: index) + } else { + // keep in queue if no executor matches, we may not have the executor available yet + index += 1 } + } - // Persist the deltas (including removed deltas) to storage after they are divvy'd up to executors. - OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) + // Persist the deltas (including removed deltas) to storage after they are divvy'd up to executors. + OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_OPERATION_REPO_DELTA_QUEUE_KEY, withValue: self.deltaQueue) - for executor in executors { - executor.cacheDeltaQueue() - } + for executor in self.executors { + executor.cacheDeltaQueue() + } - for executor in executors { - executor.processDeltaQueue(inBackground: inBackground) - } + for executor in self.executors { + executor.processDeltaQueue(inBackground: inBackground) + } - if inBackground { - OSBackgroundTaskManager.endBackgroundTask(OPERATION_REPO_BACKGROUND_TASK) - } + if inBackground { + OSBackgroundTaskManager.endBackgroundTask(OPERATION_REPO_BACKGROUND_TASK) } } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 36d8ceaff..e63b4da46 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -594,7 +594,7 @@ extension OneSignalUserManagerImpl { */ @objc public func runBackgroundTasks() { - OSOperationRepo.sharedInstance.flushDeltaQueue(inBackground: true) + OSOperationRepo.sharedInstance.addFlushDeltaQueueToDispatchQueue(inBackground: true) } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index ade8d4f19..a65947d22 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -84,7 +84,7 @@ final class OneSignalUserTests: XCTestCase { for _ in 1...4 { DispatchQueue.global().async { print("🧪 flushDeltaQueue on thread \(Thread.current)") - OSOperationRepo.sharedInstance.flushDeltaQueue() + OSOperationRepo.sharedInstance.addFlushDeltaQueueToDispatchQueue() } }