From 873ebe7633060e926543432dd9a670d759bfe9bf Mon Sep 17 00:00:00 2001 From: vahidlazio Date: Mon, 29 Apr 2024 17:00:51 +0200 Subject: [PATCH] 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 --- .../ConfidenceFeatureProvider.swift | 49 +++++++----- .../ConfidenceFeatureProviderTest.swift | 80 +++++++++++++++---- 2 files changed, 95 insertions(+), 34 deletions(-) diff --git a/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift b/Sources/ConfidenceProvider/ConfidenceFeatureProvider.swift index 9ff31daf..c4ebbcd0 100644 --- 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() + private var currentResolveTask: Task? /// 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 index bb4ea94c..67a53064 100644 --- 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