fix: Fix cancel async (#103)

* fix cancel current resolve task in new context * fix: address comments * remove sleep from tests --------- Co-authored-by: Fabrizio Demaria <[email protected]>
spotify · Apr 29, 2024 · 873ebe7 · 873ebe7
1 parent eafe8bf
commit 873ebe7
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 34 deletions.
diff --git a/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift b/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift
@@ -25,6 +25,7 @@ public class ConfidenceFeatureProvider: FeatureProvider {
     private let eventHandler = EventHandler(ProviderEvent.notReady)
     private let confidence: Confidence?
     private var cancellables = Set<AnyCancellable>()
+    private var currentResolveTask: Task<Void, Never>?
 
     /// Should not be called externally, use `ConfidenceFeatureProvider.Builder`or init with `Confidence` instead.
     init(
@@ -91,30 +92,30 @@ public class ConfidenceFeatureProvider: FeatureProvider {
 
         let context = confidence?.getContext() ?? ConfidenceTypeMapper.from(ctx: initialContext)
 
-        resolve(strategy: initializationStrategy, context: context)
+        Task {
+            await resolve(strategy: initializationStrategy, context: context)
+        }
         self.startListentingForContextChanges()
     }
 
-    private func resolve(strategy: InitializationStrategy, context: ConfidenceStruct) {
-        Task {
-            do {
-                let resolveResult = try await client.resolve(ctx: context)
-
-                // update cache with stored values
-                try await store(
-                    with: context,
-                    resolveResult: resolveResult,
-                    refreshCache: strategy == .fetchAndActivate
-                )
+    private func resolve(strategy: InitializationStrategy, context: ConfidenceStruct) async {
+        do {
+            let resolveResult = try await client.resolve(ctx: context)
 
-                // signal the provider is ready after the network request is done
-                if strategy == .fetchAndActivate {
-                    eventHandler.send(.ready)
-                }
-            } catch {
-                // We emit a ready event as the provider is ready, but is using default / cache values.
+            // update cache with stored values
+            try await store(
+                with: context,
+                resolveResult: resolveResult,
+                refreshCache: strategy == .fetchAndActivate
+            )
+
+            // signal the provider is ready after the network request is done
+            if strategy == .fetchAndActivate {
                 eventHandler.send(.ready)
             }
+        } catch {
+            // We emit a ready event as the provider is ready, but is using default / cache values.
+            eventHandler.send(.ready)
         }
     }
 
@@ -123,6 +124,7 @@ public class ConfidenceFeatureProvider: FeatureProvider {
             cancellable.cancel()
         }
         cancellables.removeAll()
+        currentResolveTask?.cancel()
     }
 
     private func store(
@@ -147,7 +149,9 @@ public class ConfidenceFeatureProvider: FeatureProvider {
         newContext: OpenFeature.EvaluationContext
     ) {
         if confidence == nil {
-            self.resolve(strategy: .fetchAndActivate, context: ConfidenceTypeMapper.from(ctx: newContext))
+            currentResolveTask = Task {
+                await resolve(strategy: .fetchAndActivate, context: ConfidenceTypeMapper.from(ctx: newContext))
+            }
             return
         }
 
@@ -163,12 +167,17 @@ public class ConfidenceFeatureProvider: FeatureProvider {
         guard let confidence = confidence else {
             return
         }
+
         confidence.contextChanges()
             .sink { [weak self] context in
                 guard let self = self else {
                     return
                 }
-                self.resolve(strategy: self.initializationStrategy, context: context)
+
+                currentResolveTask?.cancel()
+                currentResolveTask = Task {
+                    await self.resolve(strategy: .fetchAndActivate, context: context)
+                }
             }
             .store(in: &cancellables)
     }

diff --git a/Tests/ConfidenceProviderTests/ConfidenceFeatureProviderTest.swift b/Tests/ConfidenceProviderTests/ConfidenceFeatureProviderTest.swift
@@ -3,6 +3,7 @@
 import Foundation
 import Confidence
 import OpenFeature
+import Combine
 import XCTest
 
 @testable import ConfidenceProvider
@@ -24,6 +25,71 @@ class ConfidenceFeatureProviderTest: XCTestCase {
         super.setUp()
     }
 
+    func testSlowFirstResolveWillbeCancelledOnSecondResolve() async throws {
+        let expectation1 = expectation(description: "First resolve completed")
+        let expectation2 = expectation(description: "Unlock second resolve")
+        let expectation3 = expectation(description: "Third resolve completed")
+
+        class FakeClient: XCTestCase, ConfidenceResolveClient {
+            var callCount = 0
+            var resolveContexts: [ConfidenceStruct] = []
+            let expectation1: XCTestExpectation
+            let expectation2: XCTestExpectation
+            let expectation3: XCTestExpectation
+
+            init(expectation1: XCTestExpectation, expectation2: XCTestExpectation, expectation3: XCTestExpectation) {
+                self.expectation1 = expectation1
+                self.expectation2 = expectation2
+                self.expectation3 = expectation3
+                super.init(invocation: nil) // Workaround to use expectations in FakeClient
+            }
+
+            func resolve(ctx: ConfidenceStruct) async throws -> ResolvesResult {
+                callCount += 1
+                switch callCount {
+                case 1:
+                    expectation1.fulfill()
+                    if Task.isCancelled {
+                        return .init(resolvedValues: [], resolveToken: "")
+                    }
+                case 2:
+                    await fulfillment(of: [expectation2], timeout: 5.0)
+                    if Task.isCancelled {
+                        return .init(resolvedValues: [], resolveToken: "")
+                    }
+                    XCTFail("This task should be cancelled and never reach here")
+                case 3:
+                    expectation3.fulfill()
+                    if Task.isCancelled {
+                        return .init(resolvedValues: [], resolveToken: "")
+                    }
+                default: XCTFail("We expect only 3 resolve calls")
+                }
+                resolveContexts.append(ctx)
+                return .init(resolvedValues: [], resolveToken: "")
+            }
+        }
+
+        let confidence = Confidence.Builder.init(clientSecret: "").build()
+        let client = FakeClient(
+            expectation1: expectation1,
+            expectation2: expectation2,
+            expectation3: expectation3
+        )
+        let provider = ConfidenceFeatureProvider(confidence: confidence, session: nil, client: client)
+        // Initialize allows to start listening for context changes in "confidence"
+        provider.initialize(initialContext: MutableContext(targetingKey: "user1"))
+        // Let the internal "resolve" finish
+        await fulfillment(of: [expectation1], timeout: 5.0)
+        confidence.putContext(key: "new", value: ConfidenceValue(string: "value"))
+        confidence.putContext(key: "new2", value: ConfidenceValue(string: "value2"))
+        await fulfillment(of: [expectation3], timeout: 5.0)
+        expectation2.fulfill() // Allow second resolve to continue, regardless if cancelled or not
+        XCTAssertEqual(3, client.callCount)
+        XCTAssertEqual(2, client.resolveContexts.count)
+        XCTAssertEqual(confidence.getContext(), client.resolveContexts[1])
+    }
+
     func testRefresh() throws {
         var session = MockedResolveClientURLProtocol.mockedSession(flags: [:])
         let provider =
@@ -1089,18 +1155,4 @@ final class DispatchQueueFake: DispatchQueueType {
         work()
     }
 }
-
-final class DispatchQueueFakeSlow: DispatchQueueType {
-    var expectation: XCTestExpectation
-    init(expectation: XCTestExpectation) {
-        self.expectation = expectation
-    }
-    func async(execute work: @escaping @convention(block) () -> Void) {
-        Task {
-            try await Task.sleep(nanoseconds: 1 * 1_000_000_000)
-            work()
-            expectation.fulfill()
-        }
-    }
-}
 // swiftlint:enable type_body_length