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.

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: Fix FlagApplier async behaviour #70

Merged
merged 4 commits into from
Jan 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
70 changes: 39 additions & 31 deletions Sources/ConfidenceProvider/Apply/FlagApplierWithRetries.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Expand All @@ -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,
Expand All @@ -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)))
}
}

Expand All @@ -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)
}
}
}
4 changes: 2 additions & 2 deletions Sources/ConfidenceProvider/Http/HttpClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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>
}
Expand Down
67 changes: 28 additions & 39 deletions Sources/ConfidenceProvider/Http/NetworkClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Comment on lines +79 to +80
Copy link
Member Author

Choose a reason for hiding this comment

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

error is not returned from .data(), while that was the case with .dataTask(). However, .data() is marked as throwing, so the do/catch wrapping should in practice replicate the exact same behaviour of .dataTask()

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))
Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure there is a better way to achieve this

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like sleep in with Task is a fine way to schedule tasks in the newest Swift: https://www.swiftbysundell.com/articles/delaying-an-async-swift-task/

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)
}
}
}
Expand Down
47 changes: 20 additions & 27 deletions Tests/ConfidenceProviderTests/CacheDataInteractorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
}
8 changes: 4 additions & 4 deletions Tests/ConfidenceProviderTests/Helpers/HttpClientMock.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down
1 change: 1 addition & 0 deletions scripts/run_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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' \
Expand Down
Loading