From f169d907127b0c073204c3355f64443cb7299466 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Tue, 16 Jan 2024 14:00:45 +0100 Subject: [PATCH] fix: Fix FlagApplier async behaviour (#70) * chore: Make CI a bit more quiet Signed-off-by: Fabrizio Demaria * fix: apply returns after full completion of the inner tasks Signed-off-by: Fabrizio Demaria * fix: Setup async tests without Task Signed-off-by: Fabrizio Demaria * fix: Remove async lets Signed-off-by: Fabrizio Demaria --------- Signed-off-by: Fabrizio Demaria --- .../Apply/FlagApplierWithRetries.swift | 70 +++++++++++-------- .../ConfidenceProvider/Http/HttpClient.swift | 4 +- .../Http/NetworkClient.swift | 67 ++++++++---------- .../CacheDataInteractorTests.swift | 47 ++++++------- .../Helpers/HttpClientMock.swift | 8 +-- scripts/run_tests.sh | 1 + 6 files changed, 94 insertions(+), 103 deletions(-) diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift index e37c7fe9..f5813677 100644 --- a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift +++ b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift @@ -56,8 +56,8 @@ final class FlagApplierWithRetries: FlagApplier { // MARK: private private func triggerBatch() async { - async let cacheData = await cacheDataInteractor.cache - await cacheData.resolveEvents.forEach { resolveEvent in + let cacheData = await cacheDataInteractor.cache + await cacheData.resolveEvents.asyncForEach { resolveEvent in let appliesToSend = resolveEvent.events.filter { $0.status == .created } .chunk(size: 20) @@ -65,40 +65,38 @@ final class FlagApplierWithRetries: FlagApplier { return } - appliesToSend.forEach { chunk in - self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .sending) - executeApply( + await appliesToSend.asyncForEach { chunk in + await self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .sending) + await executeApply( resolveToken: resolveEvent.resolveToken, items: chunk ) { success in guard success else { - self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .created) + await self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .created) return } // Set 'sent' property of apply events to true - self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .sent) + await self.writeStatus(resolveToken: resolveEvent.resolveToken, events: chunk, status: .sent) } } } } - private func writeStatus(resolveToken: String, events: [FlagApply], status: ApplyEventStatus) { + private func writeStatus(resolveToken: String, events: [FlagApply], status: ApplyEventStatus) async { let lastIndex = events.count - 1 - events.enumerated().forEach { index, event in - Task(priority: .medium) { - var data = await self.cacheDataInteractor.setEventStatus( - resolveToken: resolveToken, - name: event.name, - status: status - ) - - if index == lastIndex { - let unsentFlagApplies = data.resolveEvents.filter { - $0.isSent == false - } - data.resolveEvents = unsentFlagApplies - try? self.storage.save(data: data) + await events.enumerated().asyncForEach { index, event in + var data = await self.cacheDataInteractor.setEventStatus( + resolveToken: resolveToken, + name: event.name, + status: status + ) + + if index == lastIndex { + let unsentFlagApplies = data.resolveEvents.filter { + $0.isSent == false } + data.resolveEvents = unsentFlagApplies + try? self.storage.save(data: data) } } } @@ -110,8 +108,8 @@ final class FlagApplierWithRetries: FlagApplier { private func executeApply( resolveToken: String, items: [FlagApply], - completion: @escaping (Bool) -> Void - ) { + completion: @escaping (Bool) async -> Void + ) async { let applyFlagRequestItems = items.map { applyEvent in AppliedFlagRequestItem( flag: applyEvent.name, @@ -126,25 +124,25 @@ final class FlagApplierWithRetries: FlagApplier { sdk: Sdk(id: metadata.name, version: metadata.version) ) - performRequest(request: request) { result in + await performRequest(request: request) { result in switch result { case .success: - completion(true) + await completion(true) case .failure(let error): self.logApplyError(error: error) - completion(false) + await completion(false) } } } private func performRequest( request: ApplyFlagsRequest, - completion: @escaping (ApplyFlagResult) -> Void - ) { + completion: @escaping (ApplyFlagResult) async -> Void + ) async { do { - try httpClient.post(path: ":apply", data: request, completion: completion) + try await httpClient.post(path: ":apply", data: request, completion: completion) } catch { - completion(.failure(handleError(error: error))) + await completion(.failure(handleError(error: error))) } } @@ -168,3 +166,13 @@ final class FlagApplierWithRetries: FlagApplier { } } } + +extension Sequence { + func asyncForEach( + _ transform: (Element) async throws -> Void + ) async rethrows { + for element in self { + try await transform(element) + } + } +} diff --git a/Sources/ConfidenceProvider/Http/HttpClient.swift b/Sources/ConfidenceProvider/Http/HttpClient.swift index 2f747604..8f0d1721 100644 --- a/Sources/ConfidenceProvider/Http/HttpClient.swift +++ b/Sources/ConfidenceProvider/Http/HttpClient.swift @@ -6,8 +6,8 @@ protocol HttpClient { func post( path: String, data: Codable, - completion: @escaping (HttpClientResult) -> Void - ) throws + completion: @escaping (HttpClientResult) async -> Void + ) async throws func post(path: String, data: Codable) async throws -> HttpClientResponse } diff --git a/Sources/ConfidenceProvider/Http/NetworkClient.swift b/Sources/ConfidenceProvider/Http/NetworkClient.swift index 9928ac4a..0ba073b4 100644 --- a/Sources/ConfidenceProvider/Http/NetworkClient.swift +++ b/Sources/ConfidenceProvider/Http/NetworkClient.swift @@ -26,14 +26,14 @@ final class NetworkClient: HttpClient { retry: Retry = .none ) { self.session = - session - ?? { - let configuration = URLSessionConfiguration.default - configuration.timeoutIntervalForRequest = timeout - configuration.httpAdditionalHeaders = defaultHeaders + session + ?? { + let configuration = URLSessionConfiguration.default + configuration.timeoutIntervalForRequest = timeout + configuration.httpAdditionalHeaders = defaultHeaders - return URLSession(configuration: configuration) - }() + return URLSession(configuration: configuration) + }() self.headers = defaultHeaders self.retry = retry @@ -44,26 +44,26 @@ final class NetworkClient: HttpClient { func post( path: String, data: Codable, - completion: @escaping (HttpClientResult) -> Void - ) throws { + completion: @escaping (HttpClientResult) async -> Void + ) async throws { let request = try buildRequest(path: path, data: data) - perform(request: request, retry: self.retry) { response, data, error in + await perform(request: request, retry: self.retry) { response, data, error in if let error { - completion(.failure(error)) + await completion(.failure(error)) return } guard let response, let data else { - completion(.failure(ConfidenceError.internalError(message: "Bad response"))) + await completion(.failure(ConfidenceError.internalError(message: "Bad response"))) return } do { let httpClientResult: HttpClientResponse = - try self.buildResponse(response: response, data: data) - completion(.success(httpClientResult)) + try self.buildResponse(response: response, data: data) + await completion(.success(httpClientResult)) } catch { - completion(.failure(error)) + await completion(.failure(error)) } } } @@ -71,40 +71,29 @@ final class NetworkClient: HttpClient { private func perform( request: URLRequest, retry: Retry, - completion: @escaping (HTTPURLResponse?, Data?, Error?) -> Void - ) { + completion: @escaping (HTTPURLResponse?, Data?, Error?) async -> Void + ) async { let retryHandler = retry.handler() let retryWait: TimeInterval? = retryHandler.retryIn() - self.session.dataTask(with: request) { data, response, error in - if let error { - if self.shouldRetry(error: error), let retryWait { - DispatchQueue.main.asyncAfter(deadline: .now() + retryWait) { - self.perform(request: request, retry: retry, completion: completion) - } - return - } else { - completion(nil, nil, error) - } - } - + do { + let (data, response) = try await self.session.data(for: request) guard let httpResponse = response as? HTTPURLResponse else { - completion(nil, nil, HttpClientError.invalidResponse) + await completion(nil, nil, HttpClientError.invalidResponse) return } - if self.shouldRetry(httpResponse: httpResponse), let retryWait { - DispatchQueue.main.asyncAfter(deadline: .now() + retryWait) { - self.perform(request: request, retry: retry, completion: completion) - } + try? await Task.sleep(nanoseconds: UInt64(retryWait * 1_000_000_000)) + await self.perform(request: request, retry: retry, completion: completion) return } - - if let data { - completion(httpResponse, data, nil) + await completion(httpResponse, data, nil) + } catch { + if self.shouldRetry(error: error), let retryWait { + try? await Task.sleep(nanoseconds: UInt64(retryWait * 1_000_000_000)) + await self.perform(request: request, retry: retry, completion: completion) } else { - let error = ConfidenceError.internalError(message: "Unable to complete request") - completion(httpResponse, nil, error) + await completion(nil, nil, error) } } } diff --git a/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift b/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift index 30ba8ace..e9425a69 100644 --- a/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift +++ b/Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift @@ -5,7 +5,7 @@ import XCTest @testable import ConfidenceProvider final class CacheDataInteractorTests: XCTestCase { - func testCacheDataInteractor_loadsEventsFromStorage() throws { + func testCacheDataInteractor_loadsEventsFromStorage() async throws { // Given prefilled storage with 10 resolve events (20 apply events in each) let prefilledCache = try CacheDataUtility.prefilledCacheData( resolveEventCount: 10, @@ -16,30 +16,25 @@ final class CacheDataInteractorTests: XCTestCase { let cacheDataInteractor = CacheDataInteractor(cacheData: prefilledCache) // Then cache data is loaded from storage - Task { - // Wrapped it in the Task in order to ensure that async code is completed before assertions - let cache = await cacheDataInteractor.cache - XCTAssertEqual(cache.resolveEvents.count, 10) - XCTAssertEqual(cache.resolveEvents.last?.events.count, 20) - } + let cache = await cacheDataInteractor.cache + XCTAssertEqual(cache.resolveEvents.count, 10) + XCTAssertEqual(cache.resolveEvents.last?.events.count, 20) } func testCacheDataInteractor_addEventToEmptyCache() async throws { // Given cache data interactor with no previously stored data let cacheDataInteractor = CacheDataInteractor(cacheData: .empty()) - Task { - let cache = await cacheDataInteractor.cache - XCTAssertEqual(cache.resolveEvents.count, 0) - } - - Task { - // When cache data add method is called - _ = await cacheDataInteractor.add(resolveToken: "token", flagName: "name", applyTime: Date()) - - // Then event is added with - let cache = await cacheDataInteractor.cache - XCTAssertEqual(cache.resolveEvents.count, 1) - } + + let cache = await cacheDataInteractor.cache + XCTAssertEqual(cache.resolveEvents.count, 0) + + + // When cache data add method is called + _ = await cacheDataInteractor.add(resolveToken: "token", flagName: "name", applyTime: Date()) + + // Then event is added with + let cache2 = await cacheDataInteractor.cache + XCTAssertEqual(cache2.resolveEvents.count, 1) } func testCacheDataInteractor_addEventToPreFilledCache() async throws { @@ -47,13 +42,11 @@ final class CacheDataInteractorTests: XCTestCase { let prefilledCacheData = try CacheDataUtility.prefilledCacheData(applyEventCount: 2) let cacheDataInteractor = CacheDataInteractor(cacheData: prefilledCacheData) - Task { - // When cache data add method is called - _ = await cacheDataInteractor.add(resolveToken: "token", flagName: "name", applyTime: Date()) + // When cache data add method is called + _ = await cacheDataInteractor.add(resolveToken: "token", flagName: "name", applyTime: Date()) - // Then event is added with - let cache = await cacheDataInteractor.cache - XCTAssertEqual(cache.resolveEvents.count, 2) - } + // Then event is added with + let cache = await cacheDataInteractor.cache + XCTAssertEqual(cache.resolveEvents.count, 2) } } diff --git a/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift b/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift index 6d325e6c..3a46a0d8 100644 --- a/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift +++ b/Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift @@ -22,13 +22,13 @@ final class HttpClientMock: HttpClient { func post( path: String, data: Codable, - completion: @escaping (ConfidenceProvider.HttpClientResult) -> Void - ) throws where T: Decodable { + completion: @escaping (ConfidenceProvider.HttpClientResult) async -> Void + ) async throws where T: Decodable { do { let result: HttpClientResponse = try handlePost(path: path, data: data) - completion(.success(result)) + await completion(.success(result)) } catch { - completion(.failure(error)) + await completion(.failure(error)) } } diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index 9d1a9077..72fc485b 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -15,6 +15,7 @@ fi (cd $root_dir && TEST_RUNNER_CLIENT_TOKEN=$test_runner_client_token TEST_RUNNER_TEST_FLAG_NAME=$2 xcodebuild \ + -quiet \ -scheme ConfidenceProvider \ -sdk "iphonesimulator" \ -destination 'platform=iOS Simulator,name=iPhone 14 Pro,OS=16.2' \