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 9 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
58 changes: 58 additions & 0 deletions Sources/BraintreeCore/RepeatingTimer.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
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(handler: { [weak self] in
self?.eventHandler?()
})
jaxdesmarais marked this conversation as resolved.
Show resolved Hide resolved
return timerSource
}()

var eventHandler: (() -> Void)?

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

private var state: State = .suspended
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we need to manage this state ourselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GCD timers can be somewhat sensitive. If you try and resume/suspend an already resumed/suspended timer, you will get a crash with a reason like:

BUG IN CLIENT OF LIBDISPATCH: Over-resume of an object

This tells us we have tried to resume an already resumed timer. We just need to balance the calls to suspend and resume, like the documentation states:

As a result, you must balance each call to dispatch_suspend with a matching call to dispatch_resume before event delivery resumes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, thanks for the background here. This makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include that link & those details in a comment?

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, added: 3be7fd8


deinit {
timer.setEventHandler {}
jaxdesmarais marked this conversation as resolved.
Show resolved Hide resolved
timer.cancel()
/*
If the timer is suspended, calling cancel without resuming
triggers a crash. This is documented here https://forums.developer.apple.com/thread/15902
jaxdesmarais marked this conversation as resolved.
Show resolved Hide resolved
scannillo marked this conversation as resolved.
Show resolved Hide resolved
*/
resume()
eventHandler = nil
}

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
56 changes: 56 additions & 0 deletions UnitTests/BraintreeCoreTests/RepeatingTimer_Tests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import XCTest
@testable import BraintreeCore

class RepeatingTimerTests: XCTestCase {

func testTimer_resumesAndSuspends() {
Copy link
Contributor

@scannillo scannillo Jun 25, 2024

Choose a reason for hiding this comment

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

Nitpick - we try to name tests like this format: test<MethodName>_{optional when<Condition>}_<ExpectedResult>

This test might be more readable as two separate tests. For ex: testResume_callsEventHandler or testSuspend_doesNotCallEventHandler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for let me know, updated here: d4bb6aa

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")

handlerCalled = false
sut.suspend()

let suspensionExpectation = self.expectation(description: "Timer should not fire after suspension")
DispatchQueue.main.asyncAfter(deadline: .now() + 3) {
suspensionExpectation.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