Backend: fixed potential race conditions introduced by OperationDispatcher.dispatchOnWorkerThread(withRandomDelay:)#1827
Merged
Conversation
8598739 to
2857360
Compare
OfferingsManager: added failing test for duplicate requestsBackend: fixed potential race conditions introduced by OperationDispatcher.dispatchOnWorkerThread(withRandomDelay:)
NachoSoto
commented
Aug 15, 2022
Contributor
Author
There was a problem hiding this comment.
The only change here is that this is removed.
NachoSoto
commented
Aug 15, 2022
Contributor
Author
There was a problem hiding this comment.
Instead of adding it directly to the queue, a new method allows to introduce a random delay, but only after the callback is in the cache.
Contributor
There was a problem hiding this comment.
Ahhhh I see, yeah, that's a much better way of doing it.
…spatcher.dispatchOnWorkerThread(withRandomDelay:)` Fixes [CSDK-391]. As described in that ticket, calling a method that introduces a random delay before performing an API request could lead to a race condition. Example: > Request 1: delay 0 seconds. Start now. > Request 2: delay 2 seconds. Waiting… > Request 1 finishes. > Request 2: 2 seconds past. There is no in progress request, so we start it again This also solves [CSDK-394]. If any part of our code (or users) call a method that would enqueue a request multiple times, we need to insert it into the callback queue right away. That's what this does: it moves the random delay to *after* the callback has been added to the `CallbackCache`.
2857360 to
62adc09
Compare
taquitos
approved these changes
Aug 16, 2022
Contributor
There was a problem hiding this comment.
Ahhhh I see, yeah, that's a much better way of doing it.
aboedo
approved these changes
Aug 16, 2022
|
|
||
| func offerings(appUserID: String, completion: ((Result<Offerings, Error>) -> Void)?) { | ||
| guard let cachedOfferings = deviceCache.cachedOfferings else { | ||
| guard let cachedOfferings = self.deviceCache.cachedOfferings else { |
Member
There was a problem hiding this comment.
[not needed for this PR]
Here's the other bug that I was talking about in slack: we assume that not having cached offerings means that we need to request them, but a request might already be in flight.
So here the check should have been something like
if cache is not stale then append to callbacks, to account for a possible request in flight (this is why we set the cache timestamp to now before we actually have the cache returned from the backend).
Then again this concurrency control predates the request caching that we have now, which automatically takes care of this bit. It'd be nice to move the bits that do setOfferingsCacheTimestampToNow to only do that after we actually have some data from the backend, but that's not needed for now.
Same thinking for CustomerInfo, they both had the same system in place
Contributor
Author
There was a problem hiding this comment.
I sent you a message about this, because I think this solves that as well.
Merging this PR for now but let me know if I misunderstood and happy to write a failing test for that scenario as well.
NachoSoto
added a commit
that referenced
this pull request
Aug 16, 2022
Follow up to [CSDK-391]. As pointed out by @aboedo, we have now 2 caching mechanisms: `CallbackCache` and `DeviceCache` timestamps. These 2 calls to set the timestamp (through `InMemoryCachedObject.updateCacheTimestamp(date:)` _before_ starting requests was there before `CallbackCache` was a thing. However, with that solution, especially after the fix in #1827, the timestamp change is unnecessary. Its purpose was to make sure that a subsequent request didn't trigger an update if it was already in progress, but again, that's already handled by `CallbackCache` now. The benefit of removing this is also that if a request fails, the timestamp cache wouldn't be an invalid `Date`, since nothing was actually fetched. In practice this isn't an issue because we clear the date on failure (See `OfferingsManager.handleOfferingsUpdateError` for example), but now we wouldn't need to rely on that.
NachoSoto
added a commit
that referenced
this pull request
Aug 17, 2022
…#1839) Follow up to [CSDK-391]. As pointed out by @aboedo, we have now 2 caching mechanisms: `CallbackCache` and `DeviceCache` timestamps. These 2 calls to set the timestamp (through `InMemoryCachedObject.updateCacheTimestamp(date:)`) _before_ starting requests was there before `CallbackCache` was a thing. However, with that solution, especially after the fix in #1827, the timestamp change is unnecessary. Its purpose was to make sure that a subsequent request didn't trigger an update if it was already in progress, but again, that's already handled by `CallbackCache` now. The benefit of removing this is also that if a request fails, the timestamp cache wouldn't be an invalid `Date`, since nothing was actually fetched. In practice this isn't an issue because we clear the date on failure (see `OfferingsManager.handleOfferingsUpdateError` for example), but now we wouldn't need to rely on that. No tests fail after removing this, as expected, since the caching behavior is covered thanks to `CallbackCache`. _Note: I did check and we can't remove these methods because they are still used elsewhere._ [CSDK-391]: https://revenuecats.atlassian.net/browse/CSDK-391?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Merged
NachoSoto
added a commit
that referenced
this pull request
Aug 19, 2022
### Bugfixes * `ErrorResponse`: don't add attribute errors to message if empty (#1844) via NachoSoto (@NachoSoto) * Purchase cancellations: unify behavior between SK1 and SK2 (#1841) via NachoSoto (@NachoSoto) * StoreKit 2: `PurchasesOrchestrator`: don't log "purchased product" if it was cancelled (#1840) via NachoSoto (@NachoSoto) * `Backend`: fixed potential race conditions introduced by `OperationDispatcher.dispatchOnWorkerThread(withRandomDelay:)` (#1827) via NachoSoto (@NachoSoto) * `DeviceCache`: `Sendable` conformance and fixed thread-safety (#1823) via NachoSoto (@NachoSoto) * Directly send delegate customer info when delegate is set (always sends cached CustomerInfo value) (#1828) via Josh Holtz (@joshdholtz) * `SystemInfo.finishTransactions`: made thread-safe (#1807) via NachoSoto (@NachoSoto) * `Purchases.shared` and `Purchases.isConfigured` are now thread-safe (#1813) via NachoSoto (@NachoSoto) * `PriceFormatterProvider: Sendable` conformance and fixed thread-safety (#1818) via NachoSoto (@NachoSoto) * `StoreKitConfigTestCase.changeStorefront`: re-enabled on iOS 16 (#1811) via NachoSoto (@NachoSoto) ### Other Changes * `DeviceCache`: no longer set cache timestamp before beginning request (#1839) via NachoSoto (@NachoSoto) * `MagicWeatherSwiftUI`: updated to use `async` APIs (#1843) via NachoSoto (@NachoSoto) * Release train (#1842) via Cesar de la Vega (@vegaro) * Adds hotfixes section to RELEASING doc (#1837) via Cesar de la Vega (@vegaro) * Update fastlane plugin (#1838) via Toni Rico (@tonidero) * Update migration doc from didReceiveUpdatedCustomerInfo to receivedUpdatedCustomerInfo (#1836) via Josh Holtz (@joshdholtz) * `PurchasesDelegate`: added test for latest cached customer info always being sent (#1830) via NachoSoto (@NachoSoto) * `CallbackCache: Sendable` conformance (#1835) via NachoSoto (@NachoSoto) * `CallbackCache`: simplified implementation using `Atomic` (#1834) via NachoSoto (@NachoSoto) * `PurchasesLogInTests`: added test to verify `logIn` updates offerings cache (#1833) via NachoSoto (@NachoSoto) * Created `PurchasesLoginTests` (#1832) via NachoSoto (@NachoSoto) * `SwiftLint`: cleaned up output (#1821) via NachoSoto (@NachoSoto) * Link to sdk reference (#1831) via aboedo (@aboedo) * `Atomic: ExpressibleByBooleanLiteral` (#1822) via NachoSoto (@NachoSoto) * `SwiftLint`: fixed build warning (#1820) via NachoSoto (@NachoSoto) * Adds an approval job that will tag the release (#1815) via Cesar de la Vega (@vegaro) * `Atomic: ExpressibleByNilLiteral` (#1804) via NachoSoto (@NachoSoto) * `PurchasesAttributionDataTests`: fixed potential race condition in flaky test (#1805) via NachoSoto (@NachoSoto) * Fixed warnings for unnecessary `try` (#1816) via NachoSoto (@NachoSoto) * Moved `AttributionFetcherError` inside `AttributionFetcher` (#1808) via NachoSoto (@NachoSoto) * Update documentation for presentCodeRedemptionSheet (#1817) via Joshua Liebowitz (@taquitos) * `Dangerfile`: added "next_release" as supported label (#1810) via NachoSoto (@NachoSoto) * PurchaseTester- Update Podfile.lock (#1814) via Joshua Liebowitz (@taquitos) * Update to latest fastlane plugin (#1802) via Toni Rico (@tonidero) * Clean up: moved `BackendIntegrationTests.xctestplan` to `TestPlans` folder (#1812) via NachoSoto (@NachoSoto) * `SK2StoreProduct`: conditionally removed `@available` workaround (#1794) via NachoSoto (@NachoSoto) * `SwiftLint`: fixed deprecation warning (#1809) via NachoSoto (@NachoSoto) * Update gems (#1791) via Joshua Liebowitz (@taquitos) * Replace usages of replace_in with replace_text_in_files action (#1803) via Toni Rico (@tonidero)
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes CSDK-391. As described in that ticket, calling a method that introduces a random delay before performing an API request could lead to a race condition. Example:
This also solves CSDK-394. If any part of our code (or users) call a method that would enqueue a request multiple times, we need to insert it into the callback queue right away.
That's what this does: it moves the random delay to after the callback has been added to the
CallbackCache.