diff --git a/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift b/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift index 36ef44424..a388fc6f9 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift @@ -36,9 +36,15 @@ public class MockOneSignalClient: NSObject, IOneSignalClient { public var lastHTTPRequest: OneSignalRequest? public var networkRequestCount = 0 public var executedRequests: [OneSignalRequest] = [] + /// Requests that have entered `execute` (including those still held / delayed). + public private(set) var startedRequests: [OneSignalRequest] = [] public var executeInstantaneously = false /// Set to true to make it unnecessary to setup mock responses for every request possible public var fireSuccessForAllRequests = false + /// When true, `execute` records the request but does not complete until `releaseHeldResponses()`. + public var holdResponses = false + + private var heldExecutions: [(request: OneSignalRequest, onSuccess: OSResultSuccessBlock, onFailure: OSClientFailureBlock)] = [] var remoteParamsResponse: [String: Any]? var shouldUseProvisionalAuthorization = false // new in iOS 12 (aka Direct to History) @@ -84,6 +90,9 @@ public class MockOneSignalClient: NSObject, IOneSignalClient { lastHTTPRequest = nil networkRequestCount = 0 executedRequests.removeAll() + startedRequests.removeAll() + heldExecutions.removeAll() + holdResponses = false executeInstantaneously = true remoteParamsResponse = nil shouldUseProvisionalAuthorization = false @@ -93,6 +102,19 @@ public class MockOneSignalClient: NSObject, IOneSignalClient { public func execute(_ request: OneSignalRequest, onSuccess successBlock: @escaping OSResultSuccessBlock, onFailure failureBlock: @escaping OSClientFailureBlock) { print("🧪 MockOneSignalClient execute called") + // Check hold + enqueue under one lock so releaseHeldResponses can't miss a callback mid-hold. + let shouldHold = lock.withLock { () -> Bool in + startedRequests.append(request) + guard holdResponses else { + return false + } + heldExecutions.append((request, successBlock, failureBlock)) + return true + } + if shouldHold { + return + } + if executeInstantaneously { finishExecutingRequest(request, onSuccess: successBlock, onFailure: failureBlock) } else { @@ -102,6 +124,19 @@ public class MockOneSignalClient: NSObject, IOneSignalClient { } } + /// Completes every request currently held by `holdResponses`, and stops holding further executes. + public func releaseHeldResponses() { + let held: [(request: OneSignalRequest, onSuccess: OSResultSuccessBlock, onFailure: OSClientFailureBlock)] = lock.withLock { + holdResponses = false + let copy = heldExecutions + heldExecutions.removeAll() + return copy + } + for item in held { + finishExecutingRequest(item.request, onSuccess: item.onSuccess, onFailure: item.onFailure) + } + } + /// Helper method to stringify the name of a request for identification and comparison private func stringify(_ request: OneSignalRequest) -> String { var stringified = request.description diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift index d2253c9d4..3f823038c 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift @@ -397,6 +397,12 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { guard !request.sentToClient else { return } + // Single-flight: don't send a second UpdateSubscription for the same model while one is in flight. + // A later coalesced request stays queued and is drained when the in-flight one finishes. + let modelId = request.subscriptionModel.modelId + guard !updateRequestQueue.contains(where: { $0 !== request && $0.sentToClient && $0.subscriptionModel.modelId == modelId }) else { + return + } guard request.prepareForExecution(newRecordsState: newRecordsState) else { return } @@ -413,13 +419,9 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { self.dispatchQueue.async { self.updateRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) - if inBackground { - OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) - } - } - if let onesignalId = OneSignalUserManagerImpl.sharedInstance.onesignalId { - if let rywToken = response?["ryw_token"] as? String + if let onesignalId = OneSignalUserManagerImpl.sharedInstance.onesignalId { + if let rywToken = response?["ryw_token"] as? String { let rywDelay = response?["ryw_delay"] as? NSNumber OSConsistencyManager.shared.setRywTokenAndDelay( @@ -431,6 +433,12 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { // handle a potential regression where ryw_token is no longer returned by API OSConsistencyManager.shared.resolveConditionsWithID(id: OSIamFetchReadyCondition.CONDITIONID) } + } + + self.executeNextPendingUpdateSubscription(for: modelId, inBackground: inBackground) + if inBackground { + OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) + } } } onFailure: { error in OneSignalLog.onesignalLog(.LL_ERROR, message: "OSSubscriptionOperationExecutor update subscription request failed with error: \(error.debugDescription)") @@ -440,6 +448,10 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { // Fail, no retry, remove from cache and queue self.updateRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) + self.executeNextPendingUpdateSubscription(for: modelId, inBackground: inBackground) + } else { + // Make the request eligible for the next flush + request.sentToClient = false } if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) @@ -447,4 +459,16 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { } } } + +} + +extension OSSubscriptionOperationExecutor { + /// Sends the oldest unsent UpdateSubscription for `modelId`, if any. Caller must be on `dispatchQueue`. + private func executeNextPendingUpdateSubscription(for modelId: String, inBackground: Bool) { + let pending = updateRequestQueue.filter { !$0.sentToClient && $0.subscriptionModel.modelId == modelId } + guard let next = pending.min(by: { $0.timestamp < $1.timestamp }) else { + return + } + executeUpdateSubscriptionRequest(next, inBackground: inBackground) + } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/SubscriptionUpdateRaceTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/SubscriptionUpdateRaceTests.swift index 88c7a014c..43c1746e9 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/SubscriptionUpdateRaceTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/SubscriptionUpdateRaceTests.swift @@ -133,6 +133,118 @@ final class SubscriptionUpdateRaceTests: XCTestCase { XCTAssertEqual(updateRequests.count, 1, "Unsent updates for the same subscription should be coalesced") } + /** + An UpdateSubscription already on the wire must not race a follow-up PATCH. + The follow-up stays queued until the in-flight request completes, then sends live state. + */ + func testInFlightUpdateBlocksFollowUpUntilCompleteThenSendsLiveState() throws { + let client = MockOneSignalClient() + client.holdResponses = true + client.fireSuccessForAllRequests = true + OneSignalCoreImpl.setSharedClient(client) + + let executor = OSSubscriptionOperationExecutor(newRecordsState: OSNewRecordsState()) + let model = makePushSubscriptionModel(notificationTypes: promptedNeverAnswered, subscriptionId: subscriptionId) + let identityModelId = UUID().uuidString + + executor.enqueueDelta(OSDelta( + name: OS_UPDATE_SUBSCRIPTION_DELTA, + identityModelId: identityModelId, + model: model, + property: "notificationTypes", + value: promptedNeverAnswered + )) + executor.processDeltaQueue(inBackground: false) + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.2) + + XCTAssertEqual(client.startedRequests.count, 1, "First UpdateSubscription should be in flight") + let firstPayload = try XCTUnwrap( + (client.startedRequests[0] as? OSRequestUpdateSubscription)?.parameters?["subscription"] as? [String: Any] + ) + XCTAssertEqual(firstPayload["notification_types"] as? Int, promptedNeverAnswered) + XCTAssertEqual(firstPayload["enabled"] as? Bool, false) + + // Permission granted while first PATCH is still in flight. + model.notificationTypes = subscribedNotificationTypes + XCTAssertTrue(model.enabled) + + executor.enqueueDelta(OSDelta( + name: OS_UPDATE_SUBSCRIPTION_DELTA, + identityModelId: identityModelId, + model: model, + property: "notificationTypes", + value: subscribedNotificationTypes + )) + executor.processDeltaQueue(inBackground: false) + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.2) + + XCTAssertEqual(client.startedRequests.count, 1, "Follow-up must wait for in-flight UpdateSubscription") + + client.releaseHeldResponses() + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5) + + XCTAssertEqual(client.startedRequests.count, 2, "Pending follow-up should send after in-flight completes") + let secondPayload = try XCTUnwrap( + (client.startedRequests[1] as? OSRequestUpdateSubscription)?.parameters?["subscription"] as? [String: Any] + ) + XCTAssertEqual(secondPayload["notification_types"] as? Int, subscribedNotificationTypes) + XCTAssertEqual(secondPayload["enabled"] as? Bool, true) + } + + /** + A retryable failure (e.g. 500/timeout) must not leave the single-flight gate locked. + The failed request becomes resendable and later updates for the model still go out. + */ + func testRetryableFailureDoesNotBlockSubsequentUpdates() throws { + let client = MockOneSignalClient() + client.fireSuccessForAllRequests = true + OneSignalCoreImpl.setSharedClient(client) + + let executor = OSSubscriptionOperationExecutor(newRecordsState: OSNewRecordsState()) + let model = makePushSubscriptionModel(notificationTypes: promptedNeverAnswered, subscriptionId: subscriptionId) + let identityModelId = UUID().uuidString + + // Fail the first update with a retryable error (mock responses are keyed by request description). + let requestKey = "OSRequestUpdateSubscription with model: \(model.modelId)" + client.setMockFailureResponseForRequest( + request: requestKey, + error: OneSignalClientError(code: 500, message: "retryable", responseHeaders: nil, response: nil, underlyingError: nil) + ) + + executor.enqueueDelta(OSDelta( + name: OS_UPDATE_SUBSCRIPTION_DELTA, + identityModelId: identityModelId, + model: model, + property: "notificationTypes", + value: promptedNeverAnswered + )) + executor.processDeltaQueue(inBackground: false) + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5) + + XCTAssertEqual(client.executedRequests.count, 1, "First update should have been attempted and failed retryably") + + // Server recovers; user accepts permission. + client.setMockResponseForRequest(request: requestKey, response: [:]) + model.notificationTypes = subscribedNotificationTypes + XCTAssertTrue(model.enabled) + + executor.enqueueDelta(OSDelta( + name: OS_UPDATE_SUBSCRIPTION_DELTA, + identityModelId: identityModelId, + model: model, + property: "notificationTypes", + value: subscribedNotificationTypes + )) + executor.processDeltaQueue(inBackground: false) + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5) + + let updateRequests = client.executedRequests.compactMap { $0 as? OSRequestUpdateSubscription } + XCTAssertEqual(updateRequests.count, 2, "Follow-up update must still send after a retryable failure") + let lastPayload = try XCTUnwrap(updateRequests.last?.parameters?["subscription"] as? [String: Any]) + XCTAssertEqual(lastPayload["notification_types"] as? Int, subscribedNotificationTypes) + XCTAssertEqual(lastPayload["enabled"] as? Bool, true) + } + // MARK: - Helpers private func makePushSubscriptionModel(notificationTypes: Int, subscriptionId: String?) -> OSSubscriptionModel {