diff --git a/Braintree.xcodeproj/project.pbxproj b/Braintree.xcodeproj/project.pbxproj index ba2d6a6196..7bc2279855 100644 --- a/Braintree.xcodeproj/project.pbxproj +++ b/Braintree.xcodeproj/project.pbxproj @@ -53,6 +53,8 @@ 428F976626727333001042E1 /* BTMockOpenURLContext.m in Sources */ = {isa = PBXBuildFile; fileRef = 428F976526727333001042E1 /* BTMockOpenURLContext.m */; }; 42FC218B25CDE0290047C49A /* BTPayPalRequest_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 42FC218A25CDE0290047C49A /* BTPayPalRequest_Tests.swift */; }; 42FC237125CE0E110047C49A /* BTPayPalCheckoutRequest_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 42FC237025CE0E110047C49A /* BTPayPalCheckoutRequest_Tests.swift */; }; + 457D7FC82C29CEC300EF6523 /* RepeatingTimer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 457D7FC72C29CEC300EF6523 /* RepeatingTimer.swift */; }; + 457D7FCA2C2A250E00EF6523 /* RepeatingTimer_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 457D7FC92C2A250E00EF6523 /* RepeatingTimer_Tests.swift */; }; 460C0C220F594AE8EE205E57 /* Pods_Tests_BraintreeCoreTests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 9239C9FE850C3587DE61A3A2 /* Pods_Tests_BraintreeCoreTests.framework */; }; 5708E0A628809AD9007946B9 /* BTJSON.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5708E0A528809AD9007946B9 /* BTJSON.swift */; }; 5708E0A828809BC6007946B9 /* BTJSONError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5708E0A728809BC6007946B9 /* BTJSONError.swift */; }; @@ -772,6 +774,8 @@ 42F75E5A24D48138007DC5E7 /* BTThreeDSecureClient_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTThreeDSecureClient_Tests.swift; sourceTree = ""; }; 42FC218A25CDE0290047C49A /* BTPayPalRequest_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTPayPalRequest_Tests.swift; sourceTree = ""; }; 42FC237025CE0E110047C49A /* BTPayPalCheckoutRequest_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTPayPalCheckoutRequest_Tests.swift; sourceTree = ""; }; + 457D7FC72C29CEC300EF6523 /* RepeatingTimer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RepeatingTimer.swift; sourceTree = ""; }; + 457D7FC92C2A250E00EF6523 /* RepeatingTimer_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RepeatingTimer_Tests.swift; sourceTree = ""; }; 463DED22C0F426A474E6D7E2 /* Pods-Tests-BraintreeCoreTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Tests-BraintreeCoreTests.release.xcconfig"; path = "Target Support Files/Pods-Tests-BraintreeCoreTests/Pods-Tests-BraintreeCoreTests.release.xcconfig"; sourceTree = ""; }; 541AEE40A1F01913E0638CC9 /* Pods-Tests-BraintreeCoreTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Tests-BraintreeCoreTests.debug.xcconfig"; path = "Target Support Files/Pods-Tests-BraintreeCoreTests/Pods-Tests-BraintreeCoreTests.debug.xcconfig"; sourceTree = ""; }; 5708E0A528809AD9007946B9 /* BTJSON.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTJSON.swift; sourceTree = ""; }; @@ -1489,8 +1493,9 @@ isa = PBXGroup; children = ( BEE2E4A628FDB64400C03FDD /* BTAnalyticsService_Tests.swift */, - 8053F05329FAB6B00076F988 /* FPTIBatchData_Tests.swift */, BEDA919F28EDDE64007441D9 /* FakeAnalyticsService.swift */, + 8053F05329FAB6B00076F988 /* FPTIBatchData_Tests.swift */, + 457D7FC92C2A250E00EF6523 /* RepeatingTimer_Tests.swift */, ); path = Analytics; sourceTree = ""; @@ -1556,6 +1561,7 @@ BEE2E4E5290080BD00C03FDD /* BTAnalyticsServiceError.swift */, BEF388C02BE52CD2000965C8 /* BTCoreAnalytics.swift */, 800E78C329E0DD5300D1B0FC /* FPTIBatchData.swift */, + 457D7FC72C29CEC300EF6523 /* RepeatingTimer.swift */, ); path = Analytics; sourceTree = ""; @@ -3245,6 +3251,7 @@ 804DC45D2B2D08FF00F17A15 /* BTConfigurationRequest.swift in Sources */, BED00CB228A57AD400D74AEC /* BTClientTokenError.swift in Sources */, BE24C67328E73E810067B11A /* BTAPIClientHTTPType.swift in Sources */, + 457D7FC82C29CEC300EF6523 /* RepeatingTimer.swift in Sources */, BE9EC0982899CF040022EC63 /* BTAPIPinnedCertificates.swift in Sources */, BEF1089029019CAE003B2FDA /* BTAnalyticsService.swift in Sources */, BEF388C32BE9340C000965C8 /* BTAPITimingDelegate.swift in Sources */, @@ -3564,6 +3571,7 @@ A9E5C23424FD6FAE00EE691F /* BTAPIClient_Tests.swift in Sources */, 8053F05729FAD4790076F988 /* Encodable+Dictionary_Tests.swift in Sources */, BEE2E4A728FDB64400C03FDD /* BTAnalyticsService_Tests.swift in Sources */, + 457D7FCA2C2A250E00EF6523 /* RepeatingTimer_Tests.swift in Sources */, A95229C424FD8EEE006F7D25 /* BTURLUtils_Tests.swift in Sources */, A908436324FD82C0004134CA /* BTBinData_Tests.swift in Sources */, A908436224FD82A9004134CA /* BTAppContextSwitcher_Tests.swift in Sources */, diff --git a/CHANGELOG.md b/CHANGELOG.md index 363194df11..435c4cf211 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * BraintreeCore * For analytics, only call `fetchOrReturnRemoteConfig()` when batch uploading, not on each analytic event enqueue * For analytics, add additional metrics on networking timing + * Fix bug causing random crashes in `BTAnalyticsService` * BraintreePayPalNativeCheckout (DEPRECATED) * **Note:** This module is deprecated and will be removed in a future version of the SDK * Add deprecated warning message to all public classes and methods diff --git a/Sources/BraintreeCore/Analytics/BTAnalyticsService.swift b/Sources/BraintreeCore/Analytics/BTAnalyticsService.swift index 8e9e2fb41c..f92c0e3e61 100644 --- a/Sources/BraintreeCore/Analytics/BTAnalyticsService.swift +++ b/Sources/BraintreeCore/Analytics/BTAnalyticsService.swift @@ -14,26 +14,35 @@ class BTAnalyticsService: Equatable { var shouldBypassTimerQueue = false // MARK: - Private Properties - + private static let events = BTAnalyticsEventsStorage() - - private let apiClient: BTAPIClient + /// Amount of time, in seconds, between batch API requests sent to FPTI - private let timerInterval = 15 + private static let timeInterval = 15 + + private static let timer = RepeatingTimer(timeInterval: timeInterval) - private static var timer: DispatchSourceTimer? + private let apiClient: BTAPIClient // MARK: - Initializer init(apiClient: BTAPIClient) { self.apiClient = apiClient + self.http = BTHTTP(authorization: apiClient.authorization, customBaseURL: Self.url) + + Self.timer.eventHandler = { [weak self] in + guard let self else { return } + Task { + await self.sendQueuedAnalyticsEvents() + } + } + Self.timer.resume() } // MARK: - Deinit deinit { - BTAnalyticsService.timer?.cancel() - BTAnalyticsService.timer = nil + Self.timer.suspend() } // MARK: - Internal Methods @@ -111,36 +120,8 @@ class BTAnalyticsService: Equatable { await BTAnalyticsService.events.append(event) - // TODO: - Refactor to make HTTP non-optional property and instantiate in init() - if self.http == nil { - self.http = BTHTTP(authorization: self.apiClient.authorization, customBaseURL: BTAnalyticsService.url) - } - - // A special value passed in by unit tests to prevent BTHTTP from actually posting - if let http = self.http, http.customBaseURL?.absoluteString == "test://do-not-send.url" { - return - } - if shouldBypassTimerQueue { await self.sendQueuedAnalyticsEvents() - return - } - - if BTAnalyticsService.timer == nil { - BTAnalyticsService.timer = DispatchSource.makeTimerSource(queue: self.http?.dispatchQueue) - BTAnalyticsService.timer?.schedule( - deadline: .now() + .seconds(self.timerInterval), - repeating: .seconds(self.timerInterval), - leeway: .seconds(1) - ) - - BTAnalyticsService.timer?.setEventHandler { - Task { - await self.sendQueuedAnalyticsEvents() - } - } - - BTAnalyticsService.timer?.resume() } } @@ -150,9 +131,9 @@ class BTAnalyticsService: Equatable { if await !BTAnalyticsService.events.isEmpty { do { let configuration = try await apiClient.fetchConfiguration() - let postParameters = await createAnalyticsEvent(config: configuration, sessionID: apiClient.metadata.sessionID, events: BTAnalyticsService.events.allValues) + let postParameters = await createAnalyticsEvent(config: configuration, sessionID: apiClient.metadata.sessionID, events: Self.events.allValues) http?.post("v1/tracking/batch/events", parameters: postParameters) { _, _, _ in } - await BTAnalyticsService.events.removeAll() + await Self.events.removeAll() } catch { return } diff --git a/Sources/BraintreeCore/Analytics/RepeatingTimer.swift b/Sources/BraintreeCore/Analytics/RepeatingTimer.swift new file mode 100644 index 0000000000..81a16555b4 --- /dev/null +++ b/Sources/BraintreeCore/Analytics/RepeatingTimer.swift @@ -0,0 +1,77 @@ +import Foundation + +final class RepeatingTimer { + + // MARK: - Private Properties + + /// Amount of time, in seconds. + private let timeInterval: Int + + private lazy var timer: DispatchSourceTimer = { + let timerSource = DispatchSource.makeTimerSource(queue: DispatchQueue.main) + timerSource.schedule( + deadline: .now() + .seconds(timeInterval), + repeating: .seconds(timeInterval), + leeway: .seconds(1) + ) + timerSource.setEventHandler { [weak self] in + self?.eventHandler?() + } + return timerSource + }() + + private var state: State = .suspended + + private enum State { + case suspended + case resumed + } + + // MARK: - Internal Properties + + var eventHandler: (() -> Void)? + + // MARK: - Initializer + + init(timeInterval: Int) { + self.timeInterval = timeInterval + } + + deinit { + timer.setEventHandler { } + timer.cancel() + // If the timer is suspended, calling cancel without resuming afterwards + // triggers a crash. This is documented here https://forums.developer.apple.com/thread/15902 + resume() + eventHandler = nil + } + + // MARK: - Internal Methods + + /* + GCD timers are sensitive to errors. It is crucial to maintain + balance between calls to `dispatch_suspend` and `dispatch_resume`. Failure to do so + results in crashes with errors similar to: + + BUG IN CLIENT OF LIBDISPATCH: Over-resume of an object + + Such errors indicate an attempt to resume an already resumed timer. According to the + documentation, each call to `dispatch_suspend` must be matched with a corresponding + call to `dispatch_resume` to ensure proper event delivery and avoid crashes. + For more information: https://developer.apple.com/library/archive/documentation/General/Conceptual/ConcurrencyProgrammingGuide/GCDWorkQueues/GCDWorkQueues.html#//apple_ref/doc/uid/TP40008091-CH103-SW8 + */ + + func resume() { + guard state != .resumed else { return } + + state = .resumed + timer.resume() + } + + func suspend() { + guard state != .suspended else { return } + + state = .suspended + timer.suspend() + } +} diff --git a/UnitTests/BraintreeCoreTests/Analytics/BTAnalyticsService_Tests.swift b/UnitTests/BraintreeCoreTests/Analytics/BTAnalyticsService_Tests.swift index 1df55c7d6b..001c947798 100644 --- a/UnitTests/BraintreeCoreTests/Analytics/BTAnalyticsService_Tests.swift +++ b/UnitTests/BraintreeCoreTests/Analytics/BTAnalyticsService_Tests.swift @@ -34,12 +34,12 @@ final class BTAnalyticsService_Tests: XCTestCase { XCTAssertEqual(mockAnalyticsHTTP.lastRequestEndpoint, "v1/tracking/batch/events") - let timestamp = self.parseTimestamp(mockAnalyticsHTTP.lastRequestParameters)! - let eventName = self.parseEventName(mockAnalyticsHTTP.lastRequestParameters) + let timestamp = parseTimestamp(mockAnalyticsHTTP.lastRequestParameters)! + let eventName = parseEventName(mockAnalyticsHTTP.lastRequestParameters) XCTAssertEqual(eventName!, "any.analytics.event") - XCTAssertGreaterThanOrEqual(timestamp, self.currentTime) - XCTAssertLessThanOrEqual(timestamp, self.oneSecondLater) + XCTAssertGreaterThanOrEqual(timestamp, currentTime) + XCTAssertLessThanOrEqual(timestamp, oneSecondLater) XCTAssertEqual(mockAnalyticsHTTP.POSTRequestCount, 1) self.validateMetadataParameters(mockAnalyticsHTTP.lastRequestParameters) diff --git a/UnitTests/BraintreeCoreTests/Analytics/RepeatingTimer_Tests.swift b/UnitTests/BraintreeCoreTests/Analytics/RepeatingTimer_Tests.swift new file mode 100644 index 0000000000..99b3eb1c12 --- /dev/null +++ b/UnitTests/BraintreeCoreTests/Analytics/RepeatingTimer_Tests.swift @@ -0,0 +1,52 @@ +import XCTest +@testable import BraintreeCore + +class RepeatingTimerTests: XCTestCase { + + func testResume_callsEventHandler() { + let sut = RepeatingTimer(timeInterval: 2) + let expectation = expectation(description: "Timer should fire") + var handlerCalled = false + + sut.eventHandler = { + handlerCalled = true + expectation.fulfill() + } + + sut.resume() + waitForExpectations(timeout: 3, handler: nil) + XCTAssertTrue(handlerCalled, "Event handler should be called after timer resumes") + } + + func testSuspend_preventsEventHandler() { + let sut = RepeatingTimer(timeInterval: 2) + let expectation = expectation(description: "Timer should not fire after suspension") + var handlerCalled = false + + sut.eventHandler = { + handlerCalled = true + } + + sut.resume() + sut.suspend() + + DispatchQueue.main.asyncAfter(deadline: .now() + 3) { + expectation.fulfill() + } + + waitForExpectations(timeout: 4, handler: nil) + XCTAssertFalse(handlerCalled, "Event handler should not be called after timer is suspended") + } + + func testDeinit() { + var sut: RepeatingTimer? = RepeatingTimer(timeInterval: 1) + sut?.resume() + + addTeardownBlock { + sut?.suspend() + } + + sut = nil + XCTAssertNil(sut, "Timer should be deallocated") + } +}