fix: Fix FlagApplier async behaviour #70
Conversation
fabriziodemaria
requested review from
mfranberg,
vahidlazio,
nicklasl and
nickybondarenko
as code owners
fabriziodemaria
force-pushed
the
expectations-tests
branch
from
73ae9ce to
f5b0d30
Compare
fabriziodemaria
changed the title
fabriziodemaria
force-pushed
the
expectations-tests
branch
from
9895bdc to
11e088c
Compare
Signed-off-by: Fabrizio Demaria <[email protected]>
fabriziodemaria
force-pushed
the
expectations-tests
branch
2 times, most recently
from
957f127 to
52d4ad8
Compare
fabriziodemaria
changed the title
fabriziodemaria
force-pushed
the
expectations-tests
branch
2 times, most recently
from
63fa498 to
3b1a06a
Compare
Signed-off-by: Fabrizio Demaria <[email protected]>
Signed-off-by: Fabrizio Demaria <[email protected]>
fabriziodemaria
force-pushed
the
expectations-tests
branch
from
3b1a06a to
223418c
Compare
| 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)) |
There was a problem hiding this comment.
Pretty sure there is a better way to achieve this
There was a problem hiding this comment.
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/
| guard let httpResponse = response as? HTTPURLResponse else { | ||
| completion(nil, nil, HttpClientError.invalidResponse) | ||
| do { | ||
| async let (data, response) = await self.session.data(for: request) |
There was a problem hiding this comment.
dataTask is not async, but this data function is, hence this change
| @@ -37,68 +37,66 @@ final class FlagApplierWithRetries: FlagApplier { | |||
|
|
|||
| public func apply(flagName: String, resolveToken: String) async { | |||
| let applyTime = Date.backport.now | |||
| let (data, added) = await cacheDataInteractor.add( | |||
| async let (data, added) = await cacheDataInteractor.add( | |||
There was a problem hiding this comment.
isn't here the problem that the add function in the cacheDataInteractor is not defined async? then if it's defined as async we can just await it normally here?
There was a problem hiding this comment.
can we try making the cache interactor add funciton async and using await when accessing cache in here ?
Signed-off-by: Fabrizio Demaria <[email protected]>
| do { | ||
| let (data, response) = try await self.session.data(for: request) |
There was a problem hiding this comment.
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()
|
Looks good 👍 |
Issues with flaky tests seem to be caused by the async
applyfunction inFlagAppliernot really waiting for the entire triggerBatch operation to finish (including network + updating the cache for apply status), before returning.Note: older context around the existing asynchronous setup: #35 (not sure how much this is relevant for this PR)