Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dispatchSource #1344

Merged
merged 19 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Braintree.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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 */; };
scannillo marked this conversation as resolved.
Show resolved Hide resolved
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 */; };
Expand Down Expand Up @@ -772,6 +774,8 @@
42F75E5A24D48138007DC5E7 /* BTThreeDSecureClient_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTThreeDSecureClient_Tests.swift; sourceTree = "<group>"; };
42FC218A25CDE0290047C49A /* BTPayPalRequest_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTPayPalRequest_Tests.swift; sourceTree = "<group>"; };
42FC237025CE0E110047C49A /* BTPayPalCheckoutRequest_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTPayPalCheckoutRequest_Tests.swift; sourceTree = "<group>"; };
457D7FC72C29CEC300EF6523 /* RepeatingTimer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RepeatingTimer.swift; sourceTree = "<group>"; };
457D7FC92C2A250E00EF6523 /* RepeatingTimer_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RepeatingTimer_Tests.swift; sourceTree = "<group>"; };
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 = "<group>"; };
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 = "<group>"; };
5708E0A528809AD9007946B9 /* BTJSON.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTJSON.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1421,6 +1425,7 @@
80F86FEF29FC2ED6003297FF /* Encodable+Dictionary.swift */,
BE7FA4C5291AC9B6003C6B63 /* Info.plist */,
62A746402B9255AC003D32FF /* PrivacyInfo.xcprivacy */,
457D7FC72C29CEC300EF6523 /* RepeatingTimer.swift */,
80A6C6182B21205900416D50 /* UIApplication+URLOpener.swift */,
8053F05829FB2F700076F988 /* URL+IsPayPal.swift */,
);
Expand Down Expand Up @@ -1961,6 +1966,7 @@
A7E93E571B601EE900957223 /* BTURLUtils_Tests.swift */,
A7B861BE1C24B19300A2422E /* BTVersion_Tests.swift */,
BE698EA528B3CDAD001D9B10 /* ConfigurationCache_Tests.swift */,
457D7FC92C2A250E00EF6523 /* RepeatingTimer_Tests.swift */,
8053F05629FAD4790076F988 /* Encodable+Dictionary_Tests.swift */,
A908436924FD88C3004134CA /* Helpers */,
A9E5C22824FD6D0800EE691F /* Info.plist */,
Expand Down Expand Up @@ -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 */,
Expand Down Expand Up @@ -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 */,
Expand Down
1 change: 0 additions & 1 deletion Demo/Application/Base/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ import BraintreeCore
if let preferences = settings.object(forKey: "PreferenceSpecifiers") as? Array<[String: Any]> {
var defaultsToRegister: [String: Any] = [:]
preferences.forEach { prefSpecification in
print(prefSpecification)
jaxdesmarais marked this conversation as resolved.
Show resolved Hide resolved
if let key = prefSpecification["Key"] as? String, prefSpecification.keys.contains("DefaultValue") {
defaultsToRegister[key] = prefSpecification["DefaultValue"]
}
Expand Down
25 changes: 18 additions & 7 deletions Demo/Application/Features/PayPalWebCheckoutViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,28 @@ class PayPalWebCheckoutViewController: PaymentButtonBaseViewController {
let payPalCheckoutButton = createButton(title: "PayPal Checkout", action: #selector(tappedPayPalCheckout))
let payPalVaultButton = createButton(title: "PayPal Vault", action: #selector(tappedPayPalVault))
let payPalAppSwitchButton = createButton(title: "PayPal App Switch", action: #selector(tappedPayPalAppSwitch))
let oneTimeCheckoutStackView = buttonsStackView(label: "1-Time Checkout", views: [
UIStackView(arrangedSubviews: [payLaterToggleLabel, payLaterToggle]),
UIStackView(arrangedSubviews: [newPayPalCheckoutToggleLabel, newPayPalCheckoutToggle]),
payPalCheckoutButton
])
let vaultStackView = buttonsStackView(label: "Vault",views: [payPalVaultButton, payPalAppSwitchButton])


let stackView = UIStackView(arrangedSubviews: [
UIStackView(arrangedSubviews: [emailLabel, emailTextField]),
buttonsStackView(label: "1-Time Checkout", views: [
UIStackView(arrangedSubviews: [payLaterToggleLabel, payLaterToggle]),
UIStackView(arrangedSubviews: [newPayPalCheckoutToggleLabel, newPayPalCheckoutToggle]),
payPalCheckoutButton
]),
buttonsStackView(label: "Vault",views: [payPalVaultButton, payPalAppSwitchButton])
oneTimeCheckoutStackView,
vaultStackView
])


NSLayoutConstraint.activate([
oneTimeCheckoutStackView.leadingAnchor.constraint(equalTo: stackView.leadingAnchor),
oneTimeCheckoutStackView.trailingAnchor.constraint(equalTo: stackView.trailingAnchor),

vaultStackView.leadingAnchor.constraint(equalTo: stackView.leadingAnchor),
vaultStackView.trailingAnchor.constraint(equalTo: stackView.trailingAnchor)
])

stackView.axis = .vertical
stackView.distribution = .fillProportionally
stackView.spacing = 25
Expand Down
55 changes: 18 additions & 37 deletions Sources/BraintreeCore/Analytics/BTAnalyticsService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -103,36 +112,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()
}
}

Expand All @@ -142,9 +123,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
}
Expand Down
70 changes: 70 additions & 0 deletions Sources/BraintreeCore/RepeatingTimer.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import Foundation

final class RepeatingTimer {
richherrera marked this conversation as resolved.
Show resolved Hide resolved
jaxdesmarais marked this conversation as resolved.
Show resolved Hide resolved

/// Amount of time, in seconds.
private let timeInterval: Int

init(timeInterval: Int) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've separate out this logic can we remove the timeInterval property from BTAnalyticsService and set the default in this class? That way the timer logic is fully contained here and we are only instantiating it in BTAnalyticsService.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, we can just hard code it to 15 in the RepeatingTimer class. BTAnalyticsService doesn't need to change the interval

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richherrera wanted to make sure you saw this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing timeInterval from class BTAnalyticsService and setting a default value in this class, we're mixing the "logic" again. Imagine we want to change the time interval from 15 seconds to 30, like in Android. I would expect to make this change in BTAnalyticsService, not RepeatingTimer. What do you think? Any ideas? What I came up with still looks like a magic number:

Suggested change
init(timeInterval: Int) {
init(timeInterval: Int = 15) {

Any suggestions on how to set the value 15 without it looking like a magic number?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that I agree with this. BTAnalyticsService only responsibility should be sending events, it should not care about how often we send events at all. On the other hand the RepeatingTimer should control all time related logic including the interval of the timer. I don't think BTAnalyticsService needs to care how often events are sent only that it's sending events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I completely agree with you. I was thinking more about making this class usable in multiple places if needed, considering that in some other context, 15 seconds might be too short. But I'm happy to make the change. Do you think it would be good to put it in the initializer?

self.timeInterval = timeInterval
}

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
}()

var eventHandler: (() -> Void)?

private enum State {
scannillo marked this conversation as resolved.
Show resolved Hide resolved
case suspended
case resumed
}

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: - GCD Timer Management

/*
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
*/
private var state: State = .suspended

func resume() {
Copy link
Contributor

@KunJeongPark KunJeongPark Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is great work Rich. I am wondering if we want to synchronize resume and suspends as well since analytics could be sent from different threads. We could use actor which would require awaits on function calls but we could also use serial dispatch queue. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent suggestion. For now, I think DispatchSource is a task that runs periodically, so we could proceed without needing actor's. Also, based on Jax's implementation, the DispatchSource is created to run on the main thread. From what I see, the task of sending analytics can be on different threads, but both only enqueue events. The task of sending them is handled by the single instance of DispatchSource.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I didn't know that this line let timerSource = DispatchSource.makeTimerSource(queue: DispatchQueue.main) ensures that the handler is run on main as well.

guard state != .resumed else { return }

state = .resumed
timer.resume()
}

func suspend() {
guard state != .suspended else { return }

state = .suspended
timer.suspend()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
65 changes: 65 additions & 0 deletions UnitTests/BraintreeCoreTests/RepeatingTimer_Tests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
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 testEventHandler_isCalled() {
scannillo marked this conversation as resolved.
Show resolved Hide resolved
let expectation = expectation(description: "EventHandler should be called")
let sut = RepeatingTimer(timeInterval: 1)

sut.eventHandler = {
expectation.fulfill()
}

sut.resume()
waitForExpectations(timeout: 2, handler: nil)
sut.suspend()
}

func testDeinit() {
var sut: RepeatingTimer? = RepeatingTimer(timeInterval: 1)
sut?.resume()

addTeardownBlock {
sut?.suspend()
}

sut = nil
XCTAssertNil(sut, "Timer should be deallocated")
}
}
Loading