NetworkOperations are asynchronous#1261
Conversation
7fcb496 to
fc45d7e
Compare
joshdholtz
left a comment
There was a problem hiding this comment.
This looks great! Just one thought/question about moving only calling self.finish in a defer as the first thing in each method 🤷♂️
There was a problem hiding this comment.
Can this defer move above the guard so self.finish() doesn't need to be explicitly called twice?
There was a problem hiding this comment.
It seems like most of the changes have two calls like this in it. I believe everything should work if we just have
defer {
self.finish()
}as the right thing in each method?
There was a problem hiding this comment.
Notice that this is inside the httpClient callback. We can't finish the operation until the HTTP request has finished, that's why we need 2 in this case (and in others). The first finish is to complete the operation in the case of an early return. Does that make sense?
There was a problem hiding this comment.
@NachoSoto I did a little sample with how defer works with callbacks in Playgrounds before commenting this and it seemed like the defer would still execute even after callback. But maybe different is happening here so its after to not rely on the defer before a callback 🤷♂️
There was a problem hiding this comment.
That callback is executing synchronously and immediately, so of course it does 😅 but our API requests are asynchronous.
Try adding a DispatchQueue.async to the completion in api.
Fixes https://app.shortcut.com/revenuecat/story/12888/api-calls-are-not-always-executed-in-order. Initial step towards fixing https://app.shortcut.com/revenuecat/story/12802/api-cache-too-aggressive. See also https://app.shortcut.com/revenuecat/story/9521/rewrite-httpclient-for-purchases-ios. - Overridden `isAsynchronous` to tell `OperationQueue` that operations run in the background - Manually calling KVO methods as required by `Operation` - Manually completing `NetworkOperation`s when all work is completed
fc45d7e to
d7b4600
Compare
|
Nope, not yet (@taquitos). This fixes a race condition in the test. Without this, the test semantics didn't really make sense cause all operations got started at once. But the caching issue is still present unfortunately, because the second GET operation gets invoked when the first one finishes (because they share the same cache key), before the POST even starts. |
Fixes [sc-12802]. ## Background Before this change, the sequence illustrated by the new test `testGetsUpdatedSubscriberInfoAfterPost` (started in #1249, with a few modifications here) would fail: - GET /subscribers/ - POST /receipts - GET /subscribers/ #1261 fixed the race condition in the test, making it so that operations are actually run serially. However, when the first GET request finished, the second one shared the cache key, so it received the (soon to be) outdated `CustomerInfo`. ## The fix Unfortunately this fix is isolated for this specific issue, and not a generalized one. A more comprehensive rewrite of `HTTPClient` is needed ([sc-9521]). I came up with several other alternatives, but what I ended up going with here involves the least amount of moving pieces. If a `GetCustomerInfoOperation` request is enqueued with `PostReceiptDataOperation` in the queue, then a new unique cache key is created to avoid past `GetCustomerInfoOperation` to share outdated data. If we simply added a random number to the `GetCustomerInfoOperation` cache key, the new test would have also passed. No other test verified the caching behavior, which is what the new `testGetSubscriberInfoDoesNotMakeTwoRequests` checks.
Fixes https://app.shortcut.com/revenuecat/story/12888/api-calls-are-not-always-executed-in-order
Initial step towards fixing https://app.shortcut.com/revenuecat/story/12802/api-cache-too-aggressive
See also https://app.shortcut.com/revenuecat/story/9521/rewrite-httpclient-for-purchases-ios
Other changes
isAsynchronousto tellOperationQueuethat operations run in the backgroundOperationNetworkOperations when all work is completed