diff --git a/CHANGELOG.md b/CHANGELOG.md index ff6fb928702..e28f6f83180 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Features + +- Add pending task queue during SDK initialization on a background queue (#7377) + ## 9.4.0 ### Breaking Changes diff --git a/Sources/Sentry/SentrySDKInternal.m b/Sources/Sentry/SentrySDKInternal.m index a22468649e7..eeab440ab4f 100644 --- a/Sources/Sentry/SentrySDKInternal.m +++ b/Sources/Sentry/SentrySDKInternal.m @@ -231,6 +231,12 @@ + (void)startWithOptions:(SentryOptions *)options andScope:scope]; [SentrySDKInternal setCurrentHub:hub]; + // Execute any pending tasks that were queued before the SDK was fully initialized. + // This handles the race condition when SDK is started on a background thread + // and methods like setUser are called before main thread initialization finishes. + // See https://github.com/getsentry/sentry-cocoa/issues/6872 + [SentryDependencyContainer.sharedInstance.pendingTaskQueue executePendingTasks]; + [SentryDependencyContainer.sharedInstance.crashWrapper startBinaryImageCache]; [SentryDependencyContainer.sharedInstance.binaryImageCache start:options.debug]; @@ -451,14 +457,23 @@ + (void)configureScope:(void (^)(SentryScope *scope))callback + (void)setUser:(SentryUser *_Nullable)user { + SentryPendingTaskQueue *pendingTaskQueue + = SentryDependencyContainer.sharedInstance.pendingTaskQueue; + + // Clear any older pending setUser tasks so only the latest value is kept. + // This also handles the race condition window between setCurrentHub and + // executePendingTasks: if setUser is called directly while an older setUser + // task is still queued, the stale task is removed before it can overwrite. + // See https://github.com/getsentry/sentry-cocoa/issues/6872 + [pendingTaskQueue removeAllWithType:SentryPendingTaskTypeSetUser]; + if (![SentrySDKInternal isEnabled]) { - // We must log with level fatal because only fatal messages get logged even when the SDK - // isn't started. We've seen multiple times that users try to set the user before starting - // the SDK, and it confuses them. Ideally, we would do something to store the user and set - // it once we start the SDK, but this is a breaking change, so we live with the workaround - // for now. - SENTRY_LOG_FATAL(@"The SDK is disabled, so setUser doesn't work. Please ensure to start " - @"the SDK before setting the user."); + // The SDK isn't fully initialized yet. Enqueue the task to be executed + // once the SDK is fully initialized. + [pendingTaskQueue + enqueue:^{ [SentrySDKInternal.currentHub setUser:user]; } + type:SentryPendingTaskTypeSetUser]; + return; } [SentrySDKInternal.currentHub setUser:user]; @@ -538,6 +553,9 @@ + (void)close { SENTRY_LOG_DEBUG(@"Starting to close SDK."); + // Clear any pending tasks that were queued before the SDK was fully initialized. + [SentryDependencyContainer.sharedInstance.pendingTaskQueue clearPendingTasks]; + [SentryDependencyContainer.sharedInstance.dispatchQueueWrapper dispatchSyncOnMainQueue:^{ #if SENTRY_TARGET_PROFILING_SUPPORTED diff --git a/Sources/Swift/Core/Helper/SentryPendingTaskQueue.swift b/Sources/Swift/Core/Helper/SentryPendingTaskQueue.swift new file mode 100644 index 00000000000..364e6e759a1 --- /dev/null +++ b/Sources/Swift/Core/Helper/SentryPendingTaskQueue.swift @@ -0,0 +1,92 @@ +@_implementationOnly import _SentryPrivate +import Foundation + +@objc @_spi(Private) +// swiftlint:disable:next missing_docs +public enum SentryPendingTaskType: Int { + // swiftlint:disable:next missing_docs + case setUser +} + +/// A thread-safe queue for pending SDK operations that are called before the SDK is fully initialized. +/// +/// When the SDK is started on a background thread, there's a window where the SDK is not fully +/// initialized but user code may call methods like `setUser`, `configureScope`, or `addBreadcrumb`. +/// This queue stores those operations and executes them once the SDK is ready. +/// +/// Tasks have a type that callers can use for deduplication via ``removeAll(type:)`` before +/// enqueueing, ensuring only the most recent value for a given type is applied during execution. +/// +/// See https://github.com/getsentry/sentry-cocoa/issues/6872 +@objc +@_spi(Private) +public final class SentryPendingTaskQueue: NSObject { + + private struct PendingTask { + let type: SentryPendingTaskType + let task: () -> Void + } + + private let lock = NSRecursiveLock() + private var pendingTasks: [PendingTask] = [] + + override public init() { + super.init() + } + + /// Adds a typed task to the pending queue. + /// - Parameters: + /// - task: A closure containing the operation to be executed. + /// - type: An enum identifying the task type. + @objc public func enqueue(_ task: @escaping () -> Void, type: SentryPendingTaskType) { + lock.synchronized { + pendingTasks.append(PendingTask(type: type, task: task)) + } + SentrySDKLog.debug("Typed task '\(type)' enqueued. SDK is not fully initialized yet.") + } + + /// Removes all pending tasks of the given type without executing them. + @objc public func removeAll(type: SentryPendingTaskType) { + lock.synchronized { + pendingTasks.removeAll { $0.type == type } + } + } + + /// Executes all pending tasks and clears the queue. + /// This should be called from the SDK initialization code after the hub is fully set up. + @objc public func executePendingTasks() { + let tasks = lock.synchronized { + let tasks = pendingTasks + pendingTasks = [] + return tasks + } + + guard !tasks.isEmpty else { + return + } + + SentrySDKLog.debug("Executing \(tasks.count) pending task(s) after SDK initialization.") + + for entry in tasks { + entry.task() + } + } + + /// Clears all pending tasks without executing them. + /// This is useful when the SDK is closed. + @objc public func clearPendingTasks() { + lock.synchronized { + pendingTasks.removeAll() + } + } + +#if SENTRY_TEST || SENTRY_TEST_CI + /// Returns the number of pending tasks. + /// Used for testing. + @objc public var pendingTaskCount: Int { + lock.synchronized { + return pendingTasks.count + } + } +#endif +} diff --git a/Sources/Swift/SentryDependencyContainer.swift b/Sources/Swift/SentryDependencyContainer.swift index fe8919d2d76..ae769f7d927 100644 --- a/Sources/Swift/SentryDependencyContainer.swift +++ b/Sources/Swift/SentryDependencyContainer.swift @@ -105,6 +105,7 @@ extension SentryFileManager: SentryFileManagerProtocol { } @objc public var dispatchQueueWrapper = Dependencies.dispatchQueueWrapper @objc public var random = Dependencies.random + @objc public var pendingTaskQueue = SentryPendingTaskQueue() @objc public var threadWrapper = Dependencies.threadWrapper @objc public var binaryImageCache = Dependencies.binaryImageCache @objc public var dateProvider: SentryCurrentDateProvider = Dependencies.dateProvider diff --git a/Tests/SentryTests/Helper/SentryPendingTaskQueueTests.swift b/Tests/SentryTests/Helper/SentryPendingTaskQueueTests.swift new file mode 100644 index 00000000000..e261127c56d --- /dev/null +++ b/Tests/SentryTests/Helper/SentryPendingTaskQueueTests.swift @@ -0,0 +1,185 @@ +@testable import Sentry +import XCTest + +class SentryPendingTaskQueueTests: XCTestCase { + + private var sut: SentryPendingTaskQueue! + + override func setUp() { + super.setUp() + sut = SentryDependencyContainer.sharedInstance().pendingTaskQueue + sut.clearPendingTasks() + } + + override func tearDown() { + sut.clearPendingTasks() + super.tearDown() + } + + func testEnqueue_whenCalled_shouldIncreasePendingTaskCount() { + // -- Arrange -- + XCTAssertEqual(0, sut.pendingTaskCount) + + // -- Act -- + sut.enqueue({ }, type: .setUser) + + // -- Assert -- + XCTAssertEqual(1, sut.pendingTaskCount) + } + + func testEnqueue_whenCalledMultipleTimes_shouldAccumulateTasks() { + // -- Arrange -- + XCTAssertEqual(0, sut.pendingTaskCount) + + // -- Act -- + sut.enqueue({ }, type: .setUser) + sut.enqueue({ }, type: .setUser) + sut.enqueue({ }, type: .setUser) + + // -- Assert -- + XCTAssertEqual(3, sut.pendingTaskCount) + } + + func testExecutePendingTasks_whenTasksExist_shouldExecuteAllTasksInOrder() { + // -- Arrange -- + var executionOrder: [Int] = [] + + sut.enqueue({ executionOrder.append(1) }, type: .setUser) + sut.enqueue({ executionOrder.append(2) }, type: .setUser) + sut.enqueue({ executionOrder.append(3) }, type: .setUser) + + // -- Act -- + sut.executePendingTasks() + + // -- Assert -- + XCTAssertEqual([1, 2, 3], executionOrder) + XCTAssertEqual(0, sut.pendingTaskCount) + } + + func testExecutePendingTasks_whenNoTasks_shouldDoNothing() { + // -- Arrange -- + XCTAssertEqual(0, sut.pendingTaskCount) + + // -- Act -- + sut.executePendingTasks() + + // -- Assert -- + XCTAssertEqual(0, sut.pendingTaskCount) + } + + func testClearPendingTasks_whenTasksExist_shouldRemoveAllWithoutExecuting() { + // -- Arrange -- + var wasExecuted = false + sut.enqueue({ wasExecuted = true }, type: .setUser) + XCTAssertEqual(1, sut.pendingTaskCount) + + // -- Act -- + sut.clearPendingTasks() + + // -- Assert -- + XCTAssertEqual(0, sut.pendingTaskCount) + XCTAssertFalse(wasExecuted) + } + + func testExecutePendingTasks_whenCalledTwice_shouldOnlyExecuteOnce() { + // -- Arrange -- + var executionCount = 0 + sut.enqueue({ executionCount += 1 }, type: .setUser) + + // -- Act -- + sut.executePendingTasks() + sut.executePendingTasks() + + // -- Assert -- + XCTAssertEqual(1, executionCount) + } + + // MARK: - removeAll(type:) + + func testRemoveAllWithType_shouldRemoveMatchingTasks() { + // -- Arrange -- + var executedValue = "" + sut.enqueue({ executedValue = "old" }, type: .setUser) + XCTAssertEqual(1, sut.pendingTaskCount) + + // -- Act -- + sut.removeAll(type: .setUser) + + // -- Assert -- + XCTAssertEqual(0, sut.pendingTaskCount) + sut.executePendingTasks() + XCTAssertEqual("", executedValue) + } + + func testRemoveAllWithType_shouldRemoveAllMatchingTasks() { + // -- Arrange -- + sut.enqueue({ }, type: .setUser) + sut.enqueue({ }, type: .setUser) + sut.enqueue({ }, type: .setUser) + XCTAssertEqual(3, sut.pendingTaskCount) + + // -- Act -- + sut.removeAll(type: .setUser) + + // -- Assert -- + XCTAssertEqual(0, sut.pendingTaskCount) + } + + func testRemoveAllThenEnqueue_shouldKeepOnlyLatestTask() { + // -- Arrange -- + var executedValues: [String] = [] + + sut.enqueue({ executedValues.append("old-user") }, type: .setUser) + + // -- Act -- + // Simulates what setUser does: clear old, enqueue new + sut.removeAll(type: .setUser) + sut.enqueue({ executedValues.append("new-user") }, type: .setUser) + + // -- Assert -- + XCTAssertEqual(1, sut.pendingTaskCount) + sut.executePendingTasks() + XCTAssertEqual(["new-user"], executedValues) + } + + func testEnqueueAndExecute_whenConcurrent_shouldBeThreadSafe() { + // -- Arrange -- + let queue1 = DispatchQueue(label: "test.queue1", attributes: .concurrent) + let queue2 = DispatchQueue(label: "test.queue2", attributes: .concurrent) + let expectation = XCTestExpectation(description: "All tasks completed") + expectation.expectedFulfillmentCount = 100 + + var executionCount = 0 + let lock = NSLock() + + // -- Act -- + for _ in 0..<50 { + queue1.async { + self.sut.enqueue({ + lock.synchronized { + executionCount += 1 + } + }, type: .setUser) + expectation.fulfill() + } + + queue2.async { + self.sut.enqueue({ + lock.synchronized { + executionCount += 1 + } + }, type: .setUser) + expectation.fulfill() + } + } + + wait(for: [expectation], timeout: 5.0) + + // Execute all pending tasks + sut.executePendingTasks() + + // -- Assert -- + XCTAssertEqual(100, executionCount) + XCTAssertEqual(0, sut.pendingTaskCount) + } +} diff --git a/Tests/SentryTests/SentrySDKInternalTests.swift b/Tests/SentryTests/SentrySDKInternalTests.swift index 1468e724c5d..3b90402fa12 100644 --- a/Tests/SentryTests/SentrySDKInternalTests.swift +++ b/Tests/SentryTests/SentrySDKInternalTests.swift @@ -229,25 +229,26 @@ class SentrySDKInternalTests: XCTestCase { XCTAssertEqual(event?.user, user) } - func testSetUserBeforeStartingSDK_LogsFatalMessage() throws { - // Arrange - let oldOutput = SentrySDKLog.getLogOutput() - - defer { - SentrySDKLog.setOutput(oldOutput) - } + func testSetUserBeforeStartingSDK_EnqueuesTaskForLaterExecution() throws { + // -- Arrange -- + let pendingTaskQueue = SentryDependencyContainer.sharedInstance().pendingTaskQueue + pendingTaskQueue.clearPendingTasks() - let logOutput = TestLogOutput() - SentrySDKLog.setLogOutput(logOutput) + // -- Act -- + let user = TestData.user + SentrySDK.setUser(user) - // Act - SentrySDK.setUser(nil) + // -- Assert -- + // Task should be enqueued + XCTAssertEqual(1, pendingTaskQueue.pendingTaskCount) - // Assert - let actualLogMessage = try XCTUnwrap(logOutput.loggedMessages.first) - let expectedLogMessage = "The SDK is disabled, so setUser doesn't work. Please ensure to start the SDK before setting the user." + // Start SDK and verify user is applied + givenSdkWithHub() + pendingTaskQueue.executePendingTasks() - XCTAssertTrue(actualLogMessage.contains(expectedLogMessage), "Expected log message to contain '\(expectedLogMessage)', but got '\(actualLogMessage)'") + let actualScope = SentrySDKInternal.currentHub().scope + let event = actualScope.applyTo(event: fixture.event, maxBreadcrumbs: 10) + XCTAssertEqual(event?.user, user) } func testSetUserAFterStartingSDK_DoesNotLogFatalMessage() { @@ -314,6 +315,126 @@ class SentrySDKInternalTests: XCTestCase { XCTAssertEqual(0, logOutput.loggedMessages.count, "Expected no log messages, but got \(logOutput.loggedMessages.count)") } + /// Tests the race condition fix for https://github.com/getsentry/sentry-cocoa/issues/6872 + /// When SDK is started on a background thread and setUser is called before main thread + /// initialization completes, the user should still be set correctly. + func testSetUser_whenSDKStartedOnBackgroundThread_shouldApplyUserAfterInit() throws { + // -- Arrange -- + SentryDependencyContainer.sharedInstance().pendingTaskQueue.clearPendingTasks() + clearTestState() + + let expectation = XCTestExpectation(description: "SDK started on background thread") + + // -- Act -- + // Start SDK on background thread + DispatchQueue.global(qos: .utility).async { + SentrySDK.start { options in + options.dsn = SentrySDKInternalTests.dsnAsString + options.removeAllIntegrations() + } + + // SDK is started but main thread init may not be complete yet + // Set user immediately (simulating the race condition) + let user = User(userId: "test-user-123") + user.email = "test@example.com" + SentrySDK.setUser(user) + + // Wait for main thread dispatch to complete + SentryDependencyContainer.sharedInstance().dispatchQueueWrapper.dispatchAsyncOnMainQueueIfNotMainThread { + expectation.fulfill() + } + } + + wait(for: [expectation], timeout: 5.0) + + // -- Assert -- + let actualScope = SentrySDKInternal.currentHub().scope + let event = actualScope.applyTo(event: Event(), maxBreadcrumbs: 10) + XCTAssertEqual("test-user-123", event?.user?.userId) + XCTAssertEqual("test@example.com", event?.user?.email) + } + + /// Tests the race condition fix between setCurrentHub and executePendingTasks. + /// When setUser is called after setCurrentHub but before executePendingTasks, + /// the stale queued task should be cleared so it doesn't overwrite the newer value. + func testSetUser_whenCalledAfterInitWithStalePendingTask_shouldClearStaleTask() throws { + // -- Arrange -- + let pendingTaskQueue = SentryDependencyContainer.sharedInstance().pendingTaskQueue + pendingTaskQueue.clearPendingTasks() + + // Simulate: setUser called before SDK init → enqueued + SentrySDK.setUser(User(userId: "old-user")) + XCTAssertEqual(1, pendingTaskQueue.pendingTaskCount) + + // Simulate: SDK init completes (setCurrentHub called, isEnabled is now true) + // but executePendingTasks hasn't run yet + givenSdkWithHub() + + // -- Act -- + // setUser called from another thread during the race window. + // This goes through the direct path (isEnabled is true) and also + // clears the stale pending setUser task via removeAll. + let newUser = User(userId: "new-user") + newUser.email = "new@example.com" + SentrySDK.setUser(newUser) + + // The stale pending task should have been removed + XCTAssertEqual(0, pendingTaskQueue.pendingTaskCount) + + // Simulate: executePendingTasks runs (nothing to replay) + pendingTaskQueue.executePendingTasks() + + // -- Assert -- + // The newer user should not have been overwritten + let actualScope = SentrySDKInternal.currentHub().scope + let event = actualScope.applyTo(event: Event(), maxBreadcrumbs: 10) + XCTAssertEqual("new-user", event?.user?.userId) + XCTAssertEqual("new@example.com", event?.user?.email) + } + + /// Tests that multiple setUser calls before SDK init are deduplicated, + /// so only the latest value is applied when pending tasks execute. + func testSetUser_whenCalledMultipleTimesBeforeInit_shouldDeduplicateAndKeepLatest() throws { + // -- Arrange -- + let pendingTaskQueue = SentryDependencyContainer.sharedInstance().pendingTaskQueue + pendingTaskQueue.clearPendingTasks() + + // -- Act -- + // Multiple setUser calls before SDK is initialized + SentrySDK.setUser(User(userId: "user-1")) + SentrySDK.setUser(User(userId: "user-2")) + SentrySDK.setUser(User(userId: "user-3")) + + // -- Assert -- + // Only one setUser task should remain due to deduplication + XCTAssertEqual(1, pendingTaskQueue.pendingTaskCount) + + // Start SDK and execute pending tasks + givenSdkWithHub() + pendingTaskQueue.executePendingTasks() + + let actualScope = SentrySDKInternal.currentHub().scope + let event = actualScope.applyTo(event: Event(), maxBreadcrumbs: 10) + XCTAssertEqual("user-3", event?.user?.userId) + } + + func testClose_whenPendingTasksExist_shouldClearThem() { + // -- Arrange -- + let pendingTaskQueue = SentryDependencyContainer.sharedInstance().pendingTaskQueue + pendingTaskQueue.clearPendingTasks() + + // Enqueue some tasks before SDK is started + SentrySDK.setUser(User(userId: "should-be-cleared")) + XCTAssertEqual(1, pendingTaskQueue.pendingTaskCount) + + // -- Act -- + SentrySDK.close() + + // -- Assert -- + // After close, the dependency container is reset, so we need to get the new instance + XCTAssertEqual(0, SentryDependencyContainer.sharedInstance().pendingTaskQueue.pendingTaskCount) + } + func testStartTransaction() throws { givenSdkWithHub()