fix: Fix FlagApplier async behaviour (#70)

* chore: Make CI a bit more quiet Signed-off-by: Fabrizio Demaria <fdema@spotify.com> * fix: apply returns after full completion of the inner tasks Signed-off-by: Fabrizio Demaria <fdema@spotify.com> * fix: Setup async tests without Task Signed-off-by: Fabrizio Demaria <fdema@spotify.com> * fix: Remove async lets Signed-off-by: Fabrizio Demaria <fdema@spotify.com> --------- Signed-off-by: Fabrizio Demaria <fdema@spotify.com>
spotify · Jan 16, 2024 · f169d90 · f169d90
1 parent 9d8d173
commit f169d90
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 103 deletions.
diff --git a/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift b/Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift
@@ -56,49 +56,47 @@ 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)
 
             guard appliesToSend.isEmpty == false else {
                 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
@@ -6,8 +6,8 @@ protocol HttpClient {
     func post<T: Decodable>(
         path: String,
         data: Codable,
-        completion: @escaping (HttpClientResult<T>) -> Void
-    ) throws
+        completion: @escaping (HttpClientResult<T>) async -> Void
+    ) async throws
 
     func post<T: Decodable>(path: String, data: Codable) async throws -> HttpClientResponse<T>
 }

diff --git 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,67 +44,56 @@ final class NetworkClient: HttpClient {
     func post<T: Decodable>(
         path: String,
         data: Codable,
-        completion: @escaping (HttpClientResult<T>) -> Void
-    ) throws {
+        completion: @escaping (HttpClientResult<T>) 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<T> =
-                    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))
             }
         }
     }
 
     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
@@ -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,44 +16,37 @@ 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 {
         // Given cache data interactor with previously stored data (1 resolve token and 2 apply event)
         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
@@ -22,13 +22,13 @@ final class HttpClientMock: HttpClient {
     func post<T>(
         path: String,
         data: Codable,
-        completion: @escaping (ConfidenceProvider.HttpClientResult<T>) -> Void
-    ) throws where T: Decodable {
+        completion: @escaping (ConfidenceProvider.HttpClientResult<T>) async -> Void
+    ) async throws where T: Decodable {
         do {
             let result: HttpClientResponse<T> = 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
@@ -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' \