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.

Sign up for GitHub

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 ConfigurationCallbackStorage Crash #1385

Merged
merged 15 commits into from
Aug 9, 2024
Merged

Fix ConfigurationCallbackStorage Crash #1385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
43de2b6
Move from DispatchQueue to actor for thread-safety
scannillo Aug 6, 2024
8f51234
CHANGELOG
scannillo Aug 6, 2024
d546f2e
Revert actor changes; don't use any thread-safety wrapper on config c…
scannillo Aug 6, 2024
8f8b5ab
WIP - Use while loop to prevent duplicate config GET requests & bool …
scannillo Aug 7, 2024
3b68a60
remove ConfigurationCallbackStorage; cleanup
jaxdesmarais Aug 7, 2024
97d769f
WIP - start updating tests
jaxdesmarais Aug 7, 2024
5c7ae5c
run configuration fetch on main thread
jaxdesmarais Aug 8, 2024
2951226
update tests; cleanup logic
jaxdesmarais Aug 8, 2024
ecf9759
combine task approach vs bool for config cache
jaxdesmarais Aug 8, 2024
cf9990c
clear existingTask at end of Task block and add weak self
KunJeongPark Aug 8, 2024
cb3bf33
address todo; update test
jaxdesmarais Aug 8, 2024
ad83d5c
remove redundant self; check config first
jaxdesmarais Aug 8, 2024
da5da37
revert cache get/ existingTask check order swap for cache read/write …
KunJeongPark Aug 8, 2024
498b348
make tests parallel and add globalActor on getConfig
KunJeongPark Aug 9, 2024
20d7a46
run fetchConfiguration for analytics on @MainActor
jaxdesmarais Aug 9, 2024
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
4 changes: 0 additions & 4 deletions Braintree.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@
80581A8C25531D0A00006F53 /* BTConfiguration+ThreeDSecure_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 80581A8B25531D0A00006F53 /* BTConfiguration+ThreeDSecure_Tests.swift */; };
80581B1D2553319C00006F53 /* BTGraphQLHTTP_SSLPinning_IntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 80581B1C2553319C00006F53 /* BTGraphQLHTTP_SSLPinning_IntegrationTests.swift */; };
806C85632B90EBED00A2754C /* PrivacyInfo.xcprivacy in Resources */ = {isa = PBXBuildFile; fileRef = 806C85622B90EBED00A2754C /* PrivacyInfo.xcprivacy */; };
807296042C41B63F0093F2AB /* ConfigurationCallbackStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807296032C41B63F0093F2AB /* ConfigurationCallbackStorage.swift */; };
8075CBEE2B1B735200CA6265 /* BTAPIRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8075CBED2B1B735200CA6265 /* BTAPIRequest.swift */; };
80842DA72B8E49EF00A5CD92 /* PrivacyInfo.xcprivacy in Resources */ = {isa = PBXBuildFile; fileRef = 80842DA62B8E49EF00A5CD92 /* PrivacyInfo.xcprivacy */; };
8087C10F2BFBACCA0020FC2E /* TokenizationKey_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8087C10E2BFBACCA0020FC2E /* TokenizationKey_Tests.swift */; };
Expand Down Expand Up @@ -857,7 +856,6 @@
8064F3942B1E4FEB0059C4CB /* BTShopperInsightsClient_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTShopperInsightsClient_Tests.swift; sourceTree = "<group>"; };
8064F3962B1E63800059C4CB /* BTShopperInsightsRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTShopperInsightsRequest.swift; sourceTree = "<group>"; };
806C85622B90EBED00A2754C /* PrivacyInfo.xcprivacy */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = PrivacyInfo.xcprivacy; sourceTree = "<group>"; };
807296032C41B63F0093F2AB /* ConfigurationCallbackStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ConfigurationCallbackStorage.swift; sourceTree = "<group>"; };
8075CBED2B1B735200CA6265 /* BTAPIRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BTAPIRequest.swift; sourceTree = "<group>"; };
80842DA62B8E49EF00A5CD92 /* PrivacyInfo.xcprivacy */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = PrivacyInfo.xcprivacy; sourceTree = "<group>"; };
8087C10E2BFBACCA0020FC2E /* TokenizationKey_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TokenizationKey_Tests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1509,7 +1507,6 @@
BE5C8C0228A2C183004F9130 /* BTConfiguration+Core.swift */,
804DC45C2B2D08FF00F17A15 /* BTConfigurationRequest.swift */,
BE698EA128AA8EEA001D9B10 /* ConfigurationCache.swift */,
807296032C41B63F0093F2AB /* ConfigurationCallbackStorage.swift */,
45EFC3962C2DBF32005E7F5B /* ConfigurationLoader.swift */,
);
path = Configuration;
Expand Down Expand Up @@ -3280,7 +3277,6 @@
800E78C429E0DD5300D1B0FC /* FPTIBatchData.swift in Sources */,
BED00CB028A579D700D74AEC /* BTClientToken.swift in Sources */,
5708E0A628809AD9007946B9 /* BTJSON.swift in Sources */,
807296042C41B63F0093F2AB /* ConfigurationCallbackStorage.swift in Sources */,
574891ED286F7ECA0020DA36 /* BTClientMetadata.swift in Sources */,
BE24C66E28E49A730067B11A /* BTAPIClientError.swift in Sources */,
BC17F9BC28D24C9E004B18CC /* BTGraphQLErrorTree.swift in Sources */,
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## unreleased
* BraintreeCore
* Fix bug where some analytics wouldn't send if `BTAPIClient` instantiated on button click
* Fix low-memory crash in ConfigurationCallbackStorage (fixes #1382)

## 6.23.2 (2024-07-30)
* BraintreePayPal
Expand Down
21 changes: 9 additions & 12 deletions Sources/BraintreeCore/BTAPIClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,23 +93,20 @@ import Foundation
/// cached on subsequent calls for better performance.
@_documentation(visibility: private)
public func fetchOrReturnRemoteConfiguration(_ completion: @escaping (BTConfiguration?, Error?) -> Void) {
configurationLoader.getConfig { [weak self] configuration, error in
guard let self else {
completion(nil, BTAPIClientError.deallocated)
return
}

if let error {
// TODO: - Consider updating all feature clients to use async version of this method?
Copy link
Contributor

Choose a reason for hiding this comment

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

Created DTMOBILES-980 for this TODO. Want to focus on tackling the crash first, then we can follow up with this change


Task { @MainActor in
Copy link
Contributor

@KunJeongPark KunJeongPark Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just making sure that these don't cause issues with nested calls to fetchOrReturnConfig.
Checking if removing @mainactor here and in the async fetchConfiguration function and just running completion handler and setupHTTPCredentials on main would preserve all the analytics calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MainActor is equivalent to running on the main dispatch queue: https://developer.apple.com/documentation/swift/mainactor. So should behave the exact same.

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think main difference for our purposes is that @mainactor can have suspension points so we wouldn't have deadlocks with nested completion handlers that call fetchOrReturnRemoteConfiguration

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just puzzled why there are differences in analytics output with this commit as opposed to what's on main or previous commits on this PR.

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we are just doing same on two call sites, so coordinating the two configurationLoader.getConfig calls.

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That test you quoted above, testCallbacks_useMainDispatchQueue(), it expects the completion handler to return on main.
So entire Task block of fetchOrReturnRemoteConfiguration doesn't need to be annotated @mainactor,
just the setupHTTPCredentials and completion invocation need to be wrapped in await MainActor.run to make this test pass, as I mentioned at top of this thread.

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't personally seen weirdness or difference in the FPTI between what was on main before, before this crash fix was implemented, after adding it, with or without MainActor annotations for fetchConfig functions.

But I trust that you saw something, there is a lot going on with sending analytics.
I do understand your concerns that fetchOrReturnRemoteConfiguration previously had expectation to be called from main. But I am not sure if this was true in case in analytics. In BTAnalyticsService, timer's event handler that calls sendQueuedAnalyticsEvents is awaited in a Task block and will run in the background thread.
This particular implementation detail was same before the crash fix.
Some completion handlers were returned on main in BTHTTP callCompletionAsync.

One small issue I can see right now is if network request fails, events will be dropped.
But these are not related to @mainactor annotation on fetchConfiguration.

My main concern is potential for more latency for fetching configuration for sendQueuedAnalyticsEvents with @mainactor annotation. But it's hard to tell.

Let's keep an eye on latency metrics and then we can revisit. What are your thoughts on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

That test you quoted above, testCallbacks_useMainDispatchQueue(), it expects the completion handler to return on main.
So entire Task block of fetchOrReturnRemoteConfiguration doesn't need to be annotated @mainactor,
just the setupHTTPCredentials and completion invocation need to be wrapped in await MainActor.run to make this test pass, as I mentioned at top of this thread.

imo, adding await MainActor.run for just the completions/setupHTTPCredentials complicates the callsite vs running the task on MainActor. There is no observed latency since getConfig is a global isolated actor already. Having a cleaner callsite is more important as there is no difference in wrapping the completions in a MainActor.run vs what exists today, but this method will also entirely be removed in a future PR.

I haven't personally seen weirdness or difference in the FPTI between what was on main before, before this crash fix was implemented, after adding it, with or without MainActor annotations for fetchConfig functions.

If you look at ticket DTMOBILES-998 you will see this same behavior where we make multiple network called and the config isn't cached properly. There are screenshots in that ticket with additional details from FPTI.

My main concerns are potential for more latency for fetching configuration for sendQueuedAnalyticsEvents with @mainactor annotation. But it's hard to tell. Let's keep an eye on latency metrics and then we can revisit.

There is clearly a need to annotate as is, removing it causes the issues we can clearly see in FPTI noted above. Removing the duplicate config call and just relying on the async method should resolve any remaining confusion. There is not additional latency added that did not exist previously.

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the screenshots. Do these extra calls happen in 6.23.2? I can check.
I believe you that you observed these, I just didn't see it myself. I am trying to figure out why.
I want to make sure I understand what @mainactor annotation is fixing.

Looks like the a lot of extra v1/config are coming from analytics sent from fetchAPITiming function.
I'll look into this. But I'm ok to leave the @mainactor annotation and observing any effects on latency,
I just want to make sure we understand why this is happening.

This is strange, I got different results from FPTI. I will post on your PR after doing a few more runs.

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I made comments on your jira ticket with screenshots. I tested 6.23.3 with @mainactor annotation as it is currently on main wit no changes, 6.23.3 without @mainactor annotation but with await MainActor.run on setHTTPCredentials and completion calls and 6.23.2 with no changes.

I didn't incorporate your changes on this branch, I am going to try that as well.

I found that there are no duplicate calls to v1/configuration in any of them, they are all fetching from cache but 6.23.3 without @mainactor annotation results in analytic events being listed more out of order, this was also true for 6.23.2, the analytics were out of order.
I just ran through each once, so I will try several times for verification.

do {
let configuration = try await configurationLoader.getConfig()
setupHTTPCredentials(configuration)
completion(configuration, nil)
} catch {
completion(nil, error)
return
}

setupHTTPCredentials(configuration)
completion(configuration, nil)
}
}

func fetchConfiguration() async throws -> BTConfiguration {
@MainActor func fetchConfiguration() async throws -> BTConfiguration {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar ❓ - why did we need these MainActor attributes?

try await configurationLoader.getConfig()
}

Expand Down
16 changes: 16 additions & 0 deletions Sources/BraintreeCore/BTHTTP.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,22 @@ class BTHTTP: NSObject, URLSessionTaskDelegate {
completion(nil, nil, error)
}
}

func get(
_ path: String,
configuration: BTConfiguration? = nil,
parameters: Encodable? = nil
) async throws -> (BTJSON?, HTTPURLResponse?) {
try await withCheckedThrowingContinuation { continuation in
get(path, configuration: configuration, parameters: parameters) { body, response, error in
if let error {
continuation.resume(throwing: error)
} else {
continuation.resume(returning: (body, response))
}
}
}
}

// TODO: - Remove when all POST bodies use Codable, instead of BTJSON/raw dictionaries
func post(
Expand Down
26 changes: 0 additions & 26 deletions Sources/BraintreeCore/Configuration/ConfigurationCallbackStorage.swift
View file
Open in desktop

This file was deleted.

79 changes: 33 additions & 46 deletions Sources/BraintreeCore/Configuration/ConfigurationLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import Foundation

@globalActor actor ConfigurationActor {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is neat! Can we add a docstring for what purpose this serves?

static let shared = ConfigurationActor()
}

class ConfigurationLoader {

// MARK: - Private Properties

private let configPath = "v1/configuration"
private let configurationCache = ConfigurationCache.shared
private let http: BTHTTP
private let pendingCompletions = ConfigurationCallbackStorage()

// MARK: - Intitializer

private var existingTask: Task<BTConfiguration, Error>?

// MARK: - Initializer

init(http: BTHTTP) {
self.http = http
Expand All @@ -35,56 +40,38 @@ class ConfigurationLoader {
/// - `BTConfiguration?`: The configuration object if it is successfully fetched or retrieved from the cache.
/// - `Error?`: An error object if fetching the configuration fails or if the instance is deallocated.
@_documentation(visibility: private)
func getConfig(completion: @escaping (BTConfiguration?, Error?) -> Void) {
@ConfigurationActor
func getConfig() async throws -> BTConfiguration {
if let existingTask {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we check the cache before bothering checking if there are any pending tasks?

@jaxdesmarais @KunJeongPark

Copy link
Contributor

@KunJeongPark KunJeongPark Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Jax had the same question. I was thinking of scenario where there may be read/write cache race condition if there is a pending task. The task includes writing into the cache.

return try await existingTask.value
}

if let cachedConfig = try? configurationCache.getFromCache(authorization: http.authorization.bearer) {
completion(cachedConfig, nil)
return
return cachedConfig
}

pendingCompletions.add(completion)

// If this is the 1st `v1/config` GET attempt, proceed with firing the network request.
// Otherwise, there is already a pending network request.
if pendingCompletions.count == 1 {
http.get(configPath, parameters: BTConfigurationRequest()) { [weak self] body, response, error in
guard let self else {
self?.notifyCompletions(nil, BTAPIClientError.deallocated)
return
}

if let error {
notifyCompletions(nil, error)
return
} else if response?.statusCode != 200 || body == nil {
notifyCompletions(nil, BTAPIClientError.configurationUnavailable)
return

let task = Task { [weak self] in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we just do existingTask = Task { [weak self] in ... instead?

Line 75 is a little confusing why we're doing the await on the task constant and not the re-assigned existingTask variable.

guard let self else {
throw BTAPIClientError.deallocated
}

defer { self.existingTask = nil }
do {
let (body, response) = try await http.get(configPath, parameters: BTConfigurationRequest())

if response?.statusCode != 200 || body == nil {
throw BTAPIClientError.configurationUnavailable
} else {
let configuration = BTConfiguration(json: body)

try? configurationCache.putInCache(authorization: http.authorization.bearer, configuration: configuration)

notifyCompletions(configuration, nil)
return
}
}
}
}

func getConfig() async throws -> BTConfiguration {
try await withCheckedThrowingContinuation { continuation in
getConfig { configuration, error in
if let error {
continuation.resume(throwing: error)
} else if let configuration {
continuation.resume(returning: configuration)
return configuration
}
} catch {
throw error
}
}
}

// MARK: - Private Methods

func notifyCompletions(_ configuration: BTConfiguration?, _ error: Error?) {
pendingCompletions.invoke(configuration, error)

existingTask = task
return try await task.value
}
}
88 changes: 42 additions & 46 deletions UnitTests/BraintreeCoreTests/Configuration/ConfigurationLoader_Tests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,89 +20,80 @@ class ConfigurationLoader_Tests: XCTestCase {
super.tearDown()
}

func testGetConfig_whenCached_returnsConfigFromCache() {
func testGetConfig_whenCached_returnsConfigFromCache() async {
let sampleJSON = ["test": "value", "environment": "fake-env1"]
try? ConfigurationCache.shared.putInCache(authorization: "development_tokenization_key", configuration: BTConfiguration(json: BTJSON(value: sampleJSON)))

let expectation = expectation(description: "Callback invoked")
sut.getConfig { configuration, error in
XCTAssertEqual(configuration?.environment, "fake-env1")
XCTAssertEqual(configuration?.json?["test"].asString(), "value")
do {
let configuration = try await sut.getConfig()
XCTAssertEqual(configuration.environment, "fake-env1")
XCTAssertEqual(configuration.json?["test"].asString(), "value")
XCTAssertNil(self.mockHTTP.lastRequestEndpoint)
expectation.fulfill()
} catch {
XCTFail("Should not fail")
}
waitForExpectations(timeout: 1)
}

func testGetConfig_performsGETWithCorrectPayload() {
func testGetConfig_performsGETWithCorrectPayload() async {
mockHTTP.stubRequest(withMethod: "GET", toEndpoint: "/v1/configuration", respondWith: [] as [Any?], statusCode: 200)

let expectation = expectation(description: "Callback invoked")
sut.getConfig { _, _ in
XCTAssertEqual(self.mockHTTP.lastRequestEndpoint, "v1/configuration")
XCTAssertEqual(self.mockHTTP.lastRequestParameters?["configVersion"] as? String, "3")
expectation.fulfill()

do {
let _ = try await sut.getConfig()
} catch {
// no-op
}

waitForExpectations(timeout: 1)
XCTAssertEqual(self.mockHTTP.lastRequestEndpoint, "v1/configuration")
XCTAssertEqual(self.mockHTTP.lastRequestParameters?["configVersion"] as? String, "3")
}

func testGetConfig_canGetRemoteConfiguration() {
func testGetConfig_canGetRemoteConfiguration() async {
mockHTTP.cannedConfiguration = BTJSON(value: ["test": true])
mockHTTP.cannedStatusCode = 200
let expectation = expectation(description: "Fetch configuration")
sut.getConfig { configuration, error in

do {
let configuration = try await sut.getConfig()
XCTAssertNotNil(configuration)
XCTAssertNil(error)
XCTAssertGreaterThanOrEqual(self.mockHTTP.GETRequestCount, 1)

guard let json = configuration?.json else { return }
guard let json = configuration.json else { return }
XCTAssertTrue(json["test"].isTrue)
expectation.fulfill()
} catch {
XCTFail("Should not fail")
}

waitForExpectations(timeout: 1)
}

func testGetConfig_whenServerRespondsWithNon200StatusCode_returnsAPIClientError() {
func testGetConfig_whenServerRespondsWithNon200StatusCode_returnsAPIClientError() async {
mockHTTP.stubRequest(
withMethod: "GET",
toEndpoint: "/client_api/v1/configuration",
respondWith: ["error_message": "Something bad happened"],
statusCode: 503
)

let expectation = expectation(description: "Callback invoked")
sut.getConfig { configuration, error in

do {
let _ = try await sut.getConfig()
} catch {
guard let error = error as NSError? else { return }
XCTAssertNil(configuration)
XCTAssertEqual(error.domain, BTAPIClientError.errorDomain)
XCTAssertEqual(error.code, BTAPIClientError.configurationUnavailable.rawValue)
XCTAssertEqual(error.localizedDescription, "The operation couldn’t be completed. Unable to fetch remote configuration from Braintree API at this time.")
expectation.fulfill()
}

waitForExpectations(timeout: 1)
}

func testGetConfig_whenNetworkHasError_returnsNetworkErrorInCallback() {
func testGetConfig_whenNetworkHasError_returnsNetworkErrorInCallback() async {
ConfigurationCache.shared.cacheInstance.removeAllObjects()
let mockError: NSError = NSError(domain: NSURLErrorDomain, code: NSURLErrorCannotConnectToHost)
mockHTTP.stubRequest(withMethod: "GET", toEndpoint: "/client_api/v1/configuration", respondWithError: mockError)

let expectation = expectation(description: "Fetch configuration")
sut.getConfig { configuration, error in
do {
let _ = try await sut.getConfig()
} catch {
// BTAPIClient fetches the config when initialized so there can potentially be 2 requests here
XCTAssertLessThanOrEqual(self.mockHTTP.GETRequestCount, 2)
XCTAssertNil(configuration)
XCTAssertEqual(error as NSError?, mockError)
expectation.fulfill()
}

waitForExpectations(timeout: 1)
}

func testGetConfig_returnsConfiguration() async throws {
mockHTTP.cannedConfiguration = BTJSON(value: ["test": true])
mockHTTP.cannedStatusCode = 200
Expand Down Expand Up @@ -132,11 +123,16 @@ class ConfigurationLoader_Tests: XCTestCase {
}
}

func testGetConfig_whenCalledInQuickSequence_onlySendsOneNetworkRequest() {
sut.getConfig() { _, _ in }
sut.getConfig() { _, _ in }
sut.getConfig() { _, _ in }
sut.getConfig() { _, _ in }
func testGetConfig_whenCalledInQuickSequence_onlySendsOneNetworkRequest() async {
do {
async let functionOne = sut.getConfig()
async let two = sut.getConfig()
async let three = sut.getConfig()
async let four = sut.getConfig()
let _ = try await (functionOne, two, three, four)
KunJeongPark marked this conversation as resolved.
Show resolved Hide resolved
} catch {
// no op
}

XCTAssertEqual(mockHTTP.GETRequestCount, 1)
}
Expand Down
12 changes: 1 addition & 11 deletions UnitTests/BraintreeCoreTests/Configuration/MockConfigurationLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,11 @@ class MockConfigurationLoader: ConfigurationLoader {
super.init(http: http)
}

override func getConfig(completion: @escaping (BTConfiguration?, Error?) -> Void) {
if let error = mockError {
completion(nil, error)
} else {
completion(mockConfig, nil)
}
}

override func getConfig() async throws -> BTConfiguration {
if let error = mockError {
throw error
} else if let config = mockConfig {
return config
} else {
throw BTAPIClientError.configurationUnavailable
return BTConfiguration(json: nil)
}
}
}
Loading