Add jitter and extra cache for background processes #366

aboedo · 2020-10-05T19:18:46Z

Apps opening from push notifications and potentially widgets may cause unnecessary strain on the backend by making calls simultaneously on all devices.

To alleviate the backend strain, we want to introduce a small, random delay to cache updates performed while the app is in the background. This will distribute requests over time so that the backend isn’t hit at once.

We’re also going to be more aggressive with the cache while the app is in the background, moving it from 5 mins to 1 day.

The value of 5 mins will still be used if the app is in the foreground.

Ruleset:

if app is opened and in the foreground
- if cache is older than 5 mins
  - update cache
if app is opened and in the background:
- if cache is older than one day:
  - update cache (with jitter)

…running in the background.

aboedo · 2020-10-05T19:19:55Z

Purchases/Caching/RCDeviceCache.m


 @interface RCDeviceCache ()

 @property (nonatomic) NSUserDefaults *userDefaults;
 @property (nonatomic) NSNotificationCenter *notificationCenter;
 @property (nonatomic, nonnull) RCInMemoryCachedObject<RCOfferings *> *offeringsCachedObject;
-@property (nonatomic, nullable) NSDate *purchaserInfoCachesLastUpdated;


We were storing this value in memory before. This doesn't work well for widgets, since the process for the widget will likely be killed before the next refresh, so it'll always refresh cache.

aboedo · 2020-10-05T19:20:01Z

Purchases/Caching/RCDeviceCache.m

-#import "RCLogUtils.h"
-#import "NSDictionary+RCExtensions.h"


unused imports

aboedo · 2020-10-05T19:20:26Z

Purchases/Caching/RCDeviceCache.h

@@ -33,21 +33,21 @@ NS_ASSUME_NONNULL_BEGIN

 - (void)cachePurchaserInfo:(NSData *)data forAppUserID:(NSString *)appUserID;

- (BOOL)isPurchaserInfoCacheStale;
+- (BOOL)isPurchaserInfoCacheStaleForAppUserID:(NSString *)appUserID isAppBackgrounded:(BOOL)isAppBackgrounded;


not a huge fan of how this API is turning out, it feels overly complicated. But I didn't want to perform a major refactor for this.

aboedo · 2020-10-05T19:20:38Z

Purchases/Caching/RCDeviceCache.m

@@ -26,11 +23,12 @@ @interface RCDeviceCache ()
 NSString *RCLegacyGeneratedAppUserDefaultsKey = RC_CACHE_KEY_PREFIX @".appUserID";
 NSString *RCAppUserDefaultsKey = RC_CACHE_KEY_PREFIX @".appUserID.new";
 NSString *RCPurchaserInfoAppUserDefaultsKeyBase = RC_CACHE_KEY_PREFIX @".purchaserInfo.";
+NSString *RCPurchaserInfoLastUpdatedKeyBase = RC_CACHE_KEY_PREFIX @".purchaserInfoLastUpdated.";


new userDefaults entry

aboedo · 2020-10-05T19:22:31Z

Purchases/Caching/RCDeviceCache.m

@@ -125,34 +122,55 @@ - (void)cachePurchaserInfo:(NSData *)data forAppUserID:(NSString *)appUserID {
    @synchronized (self) {
        [self.userDefaults setObject:data
                              forKey:[self purchaserInfoUserDefaultCacheKeyForAppUserID:appUserID]];
-        [self setPurchaserInfoCacheTimestampToNow];
+        [self setPurchaserInfoCacheTimestampToNowForAppUserID:appUserID];


this didn't used to be tied to an appUserID, but I tied it now for the sake of consistency.

aboedo · 2020-10-05T19:24:31Z

Purchases/Caching/RCDeviceCache.m

@@ -163,8 +181,9 @@ - (void)cacheOfferings:(RCOfferings *)offerings {
    [self.offeringsCachedObject cacheInstance:offerings];
 }

- (BOOL)isOfferingsCacheStale {
-    return self.offeringsCachedObject.isCacheStale;
+- (BOOL)isOfferingsCacheStaleWithIsAppBackgrounded:(BOOL)isAppBackgrounded {


having a different cache might not be strictly necessary for offerings, really - it's unlikely that you'd need offerings while the app is running in the background. can't hurt, though.

aboedo · 2020-10-05T19:28:44Z

Purchases/Public/RCPurchases.m

-                [self.operationDispatcher dispatchOnWorkerThread:^{
+                [self.operationDispatcher dispatchOnWorkerThreadWithRandomDelay:NO :^{


the value for randomDelay is always isBackgrounded, it might make sense to have the operationDispatcher depend on systemInfo and get the value without a param

I actually think the param is better. That way we don't add logic to the operationDispatcher that depends on the state of the system. The operation dispatcher only knows what it means to add a random delay, it doesn't care if the app is backgrounded or not. Making operationDispatcher depend on the system info also means that you don't have flexibility to do the random delay only for getOfferings or getPurchaserInfo

aboedo · 2020-10-05T19:29:34Z

Purchases/Public/RCPurchases.m

+    [self.systemInfo isApplicationBackgroundedWithCompletion:^(BOOL isAppBackgrounded) {
+        RCPurchaserInfo *infoFromCache = [self readPurchaserInfoFromCache];
+        if (infoFromCache) {
+            RCDebugLog(@"Vending purchaserInfo from cache");
+            CALL_IF_SET_ON_MAIN_THREAD(completion, infoFromCache, nil);
+            if ([self.deviceCache isPurchaserInfoCacheStaleForAppUserID:self.appUserID isAppBackgrounded:isAppBackgrounded]) {
+                RCDebugLog(@"Cache is stale, updating caches");
+                [self fetchAndCachePurchaserInfoWithCompletion:nil isAppBackgrounded:isAppBackgrounded];
+            }
+        } else {
+            RCDebugLog(@"No cached purchaser info, fetching");
+            [self fetchAndCachePurchaserInfoWithCompletion:completion isAppBackgrounded:isAppBackgrounded];


really don't love making these even more complex, but I couldn't think of a way around it given that the isAppBackgrounded method is async.

aboedo · 2020-10-05T19:30:59Z

Purchases/Public/RCPurchases.m

+    [self.systemInfo isApplicationBackgroundedWithCompletion:^(BOOL isAppBackgrounded) {
+        if ([self.deviceCache isPurchaserInfoCacheStaleForAppUserID:self.appUserID isAppBackgrounded:isAppBackgrounded]) {
+            RCDebugLog(@"PurchaserInfo cache is stale, updating caches");
+            [self fetchAndCachePurchaserInfoWithCompletion:nil isAppBackgrounded:isAppBackgrounded];
+        }
+        if ([self.deviceCache isOfferingsCacheStaleWithIsAppBackgrounded:isAppBackgrounded]) {
+            RCDebugLog(@"Offerings cache is stale, updating caches");
+            [self updateOfferingsCache:nil isAppBackgrounded:isAppBackgrounded];
+        }
+    }];


would love to move all this stuff into a new class, but it feels too risky for this particular feature

aboedo · 2020-10-05T19:31:34Z

Purchases/Public/RCPurchases.m

-                                          CALL_IF_SET_ON_MAIN_THREAD(completion, info, error);
-                                      }];
+    [self.deviceCache setPurchaserInfoCacheTimestampToNowForAppUserID:appUserID];
+    [self.operationDispatcher dispatchOnWorkerThreadWithRandomDelay:isAppBackgrounded :^{


this API came out a bit weird - note the nameless second param that just starts with :

…lastUpdatedAt from userDefaults

aboedo · 2020-10-06T14:29:39Z

PurchasesTests/Caching/DeviceCacheTests.swift

+    func testNewDeviceCacheInstanceWithExistingValidPurchaserInfoCacheIsntStale() {
+        let mockNotificationCenter = MockNotificationCenter()
+        let appUserID = "myUser"
+        let fourMinutesAgo = Calendar.current.date(byAdding: .minute, value: -4, to: Date())
+        mockUserDefaults.mockValues["com.revenuecat.userdefaults.purchaserInfoLastUpdated.\(appUserID)"] = fourMinutesAgo
+        self.deviceCache = RCDeviceCache(mockUserDefaults,
+                                         offeringsCachedObject: nil,
+                                         notificationCenter: mockNotificationCenter)
+
+        expect(self.deviceCache.isPurchaserInfoCacheStale(forAppUserID: appUserID,
+                                                          isAppBackgrounded: false)) == false
+    }
+
+    func testNewDeviceCacheInstanceWithExistingInvalidPurchaserInfoCacheIsStale() {
+        let mockNotificationCenter = MockNotificationCenter()
+        let appUserID = "myUser"
+        let fourDaysAgo = Calendar.current.date(byAdding: .day, value: -4, to: Date())
+        mockUserDefaults.mockValues["com.revenuecat.userdefaults.purchaserInfoLastUpdated.\(appUserID)"] = fourDaysAgo
+        self.deviceCache = RCDeviceCache(mockUserDefaults,
+                                         offeringsCachedObject: nil,
+                                         notificationCenter: mockNotificationCenter)
+
+        expect(self.deviceCache.isPurchaserInfoCacheStale(forAppUserID: appUserID,
+                                                          isAppBackgrounded: false)) == true
+    }
+
+    func testNewDeviceCacheInstanceWithNoCachedPurchaserInfoCacheIsStale() {
+        let mockNotificationCenter = MockNotificationCenter()
+        let appUserID = "myUser"
+        self.deviceCache = RCDeviceCache(mockUserDefaults,
+                                         offeringsCachedObject: nil,
+                                         notificationCenter: mockNotificationCenter)
+
+        expect(self.deviceCache.isPurchaserInfoCacheStale(forAppUserID: appUserID,
+                                                          isAppBackgrounded: false)) == true


these would have failed in develop since we used to have the purchaserInfoUpdatedAt value in memory and not userDefaults

…dBecomeActive unless cache is stale

vegaro · 2020-10-07T21:15:41Z

Purchases/Public/RCPurchases.m

@@ -299,7 +299,7 @@ - (instancetype)initWithAppUserID:(nullable NSString *)appUserID

        [self.systemInfo isApplicationBackgroundedWithCompletion:^(BOOL isBackgrounded) {
            if (!isBackgrounded) {
-                [self.operationDispatcher dispatchOnWorkerThread:^{
+                [self.operationDispatcher dispatchOnWorkerThreadWithRandomDelay:NO block:^{


What do you think about keeping around dispatchOnWorkerThread in operationDispatcher and make it call dispatchOnWorkerThreadWithRandomDelay:NO. I like it more since most of the times we will be passing NO.

yeah, I'll separate into two methods. I'd started by doing that, then I moved to having the randomDelay be optional, then the optional randomDelay looked great in swift but ported very poorly to obj-c, so I killed the optionality. I'll go back to the start.

ok, I was trying this and remembered why I ended up going with the param: it's so that callers don't have to choose between methods, since almost every time they pass in the value of isBackgrounded.

So if there are two separate methods, you'd have to do if isBackgrounded dispatchWithRandomDelay else dispatch.
note that we almost every time pass isBackgrounded. even in this line, we're passing NO because we know that isBackgrounded is false, because of the if in the line right above. But we could just as easily pass isBackgrounded instead.

Purchases/Public/RCPurchases.m

vegaro · 2020-10-07T21:39:13Z

Purchases/Public/RCPurchases.m

    [self.deviceCache setOfferingsCacheTimestampToNow];
-    __weak typeof(self) weakSelf = self;


I remember you added this a while ago. We don't need it anymore?

I don't think we needed it in the first place - I believe I made a mistake in thinking that there was a retention cycle there, but really, the block is used only here and the completion block isn't stored elsewhere, so as soon as the completion is executed, it dies and the block is released, then the instance is ready to be released.

The case handled here is not trivial, though:

if you call reset after calling offerings and the callback hasn't returned:

if you're doing this weak referencing (current case), the purchases instance is killed, it no longer holds on to the completion block, and completion isn't called.

if you're doing this with strong referencing (it's the default, and what we do for almost all other methods), the purchases instance lives until the completion block is called, which might lead to some unexpected behavior if there are any values in memory for the instance that are outdated after the reset.

I think we should at some point actually audit this behavior and check out what happens after a reset with some of these completion blocks.

As of right now, I'm not sure completion not returning is better or worse than it returning but potentially messing up if there's anything outdated in memory from the previous purchases instance.

PurchasesCoreSwift/Misc/OperationDispatcher.swift

vegaro · 2020-10-07T21:54:50Z

PurchasesTests/Caching/DeviceCacheTests.swift

        expect(self.mockUserDefaults.mockValues["com.revenuecat.userdefaults.purchaserInfo.cesar"] as? Data)
            .to(equal(data))
-        expect(self.deviceCache.cachedPurchaserInfoData(forAppUserID: "cesar")).to(equal(data))
-        expect(self.mockUserDefaults.setObjectForKeyCalledValue)


We don't want to check for this anymore?

the check on 153 is redundant with the one that's right above it, and the check in 154 actually assumes that the last thing you store is the purchaserInfo, which breaks when you store the purchaserInfoUpdatedDate

vegaro · 2020-10-07T22:02:26Z

PurchasesTests/Caching/DeviceCacheTests.swift

+
+        expect(self.deviceCache.isPurchaserInfoCacheStale(forAppUserID: appUserID,
+                                                          isAppBackgrounded: false)) == false
+    }


What's the difference between this test and testIsPurchaserInfoCacheStaleForForeground. Also, in this test you're calling mockUserDefaults.mockValues but in the other tests you are setting it through setPurchaserInfoCacheTimestamp

I'll take any naming suggestions I can get, but here's what they're supposed to check:

testIsPurchaserInfoCacheStaleForForeground:
test that the value of isPurchaserInfoCacheStale works correctly when the app is in the foreground.

create a cached purchaser info, but stale date.

value should be true

update the purchaserInfo cache date, with a recent value

value should be false.

testNewDeviceCacheInstanceWithExistingValidPurchaserInfoCacheIsntStale is supposed to ensure that new instances won't return that the value is stale unless it actually is. This is because we previously were storing the lastUpdated value in memory, so this would have had a different result before this PR.

store a value for lastUpdated, that's recent

check that it isn't considered stale

As for using the two mechanisms to mock, they can be unified. I had just forgotten that you could set it directly through the protected method.
Using both mechanisms might be better coverage? but less consistent. I'm happy to unify.

it's ok, both mechanisms work. I wasn't sure I was missing somehting. Thanks for the explanation

vegaro · 2020-10-07T22:04:21Z

PurchasesTests/Caching/DeviceCacheTests.swift

+        mockCachedObject.updateCacheTimestamp(with: cacheDateValidForBoth)
+        expect(self.deviceCache.isOfferingsCacheStale(withIsAppBackgrounded: false)) == false
+        expect(self.deviceCache.isOfferingsCacheStale(withIsAppBackgrounded: true)) == false
+    }


I feel we need a test for the migration case where the purchaser info timestamp is nil, but the cached purchaser info is not. This can happen for people updating from an older version. I don't think it's covered in any of these tests

vegaro · 2020-10-07T22:07:14Z

PurchasesTests/SubscriberAttributes/PurchasesSubscriberAttributesTests.swift

        expect(self.mockBackend.invokedGetSubscriberDataCount) == 1
-        expect(self.mockSubscriberAttributesManager.invokedSyncAttributesForAllUsersCount) == 0
+        expect(self.mockDeviceCache.cachePurchaserInfoCount) == 1


Isn't this line the same as the next one?

they're actually different, although the naming could be improved - one is for the read method and the other one for the write

…heStaleForAppUserID

vegaro · 2020-10-09T19:46:11Z

Purchases/Caching/RCDeviceCache.m

    NSTimeInterval timeSinceLastCheck = -[purchaserInfoCachesLastUpdated timeIntervalSinceNow];
    int cacheDurationInSeconds = [self cacheDurationInSecondsWithIsAppBackgrounded:isAppBackgrounded];
-    return !(purchaserInfoCachesLastUpdated != nil && timeSinceLastCheck < cacheDurationInSeconds);
+    return timeSinceLastCheck >= cacheDurationInSeconds;


much easier to read

* added a separate value for cache duration to be used when the app is running in the background. * added jitter for cache updates that start with the app backgrounded. * added purchaserInfoLastUpdatedAt to userDefaults * fixed missing param * fixed some tests * fixed more tests * fixed another test * cleaned up the interface for dispatchWithRandomDelay * added tests to ensure that newly-created device cache instances read lastUpdatedAt from userDefaults * added more tests for device cache * even more device cache tests * added some new test cases to purchasesTests * added test to ensure we don't automatically fetch purchaser info onDidBecomeActive unless cache is stale * updated maxJitterInSeconds to 5 * formatting of * _Nullable * simplified conditions and improved nil handling in isPurchaserInfoCacheStaleForAppUserID * small cleanup * added test case for migration

aboedo added 7 commits October 2, 2020 15:33

added a separate value for cache duration to be used when the app is …

7a1cd4e

…running in the background.

added jitter for cache updates that start with the app backgrounded.

2008fd6

added purchaserInfoLastUpdatedAt to userDefaults

249e032

fixed missing param

ff4af1e

fixed some tests

f6ca9b9

fixed more tests

6c0baf1

fixed another test

b42704c

aboedo self-assigned this Oct 5, 2020

aboedo commented Oct 5, 2020

View reviewed changes

aboedo added 2 commits October 6, 2020 11:02

cleaned up the interface for dispatchWithRandomDelay

23660b2

added tests to ensure that newly-created device cache instances read …

268bb07

…lastUpdatedAt from userDefaults

aboedo commented Oct 6, 2020

View reviewed changes

aboedo added 4 commits October 6, 2020 11:41

added more tests for device cache

f301e32

even more device cache tests

b0f4a24

added some new test cases to purchasesTests

4dfd0ee

added test to ensure we don't automatically fetch purchaser info onDi…

50240f6

…dBecomeActive unless cache is stale

aboedo marked this pull request as ready for review October 6, 2020 19:57

aboedo requested a review from vegaro October 6, 2020 19:58

vegaro reviewed Oct 7, 2020

View reviewed changes

aboedo added this to the 3.7.4 milestone Oct 8, 2020

aboedo added 5 commits October 8, 2020 19:20

updated maxJitterInSeconds to 5

9e3a02a

formatting of * _Nullable

724a491

simplified conditions and improved nil handling in isPurchaserInfoCac…

8680bce

…heStaleForAppUserID

small cleanup

3904ea4

added test case for migration

15bbf4a

vegaro reviewed Oct 9, 2020

View reviewed changes

vegaro approved these changes Oct 9, 2020

View reviewed changes

aboedo merged commit 38d16aa into develop Oct 9, 2020

aboedo deleted the feature/jitter_and_extra_cache_for_background_processes branch October 9, 2020 21:50

aboedo mentioned this pull request Oct 9, 2020

Version 3.7.4 #368

Closed

aboedo mentioned this pull request Jul 13, 2021

Release: 3.12.0 #648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jitter and extra cache for background processes #366

Add jitter and extra cache for background processes #366

aboedo commented Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 5, 2020 •

edited

aboedo Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 5, 2020

vegaro Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 5, 2020

aboedo Oct 6, 2020

vegaro Oct 7, 2020

aboedo Oct 8, 2020

aboedo Oct 8, 2020

vegaro Oct 7, 2020

aboedo Oct 8, 2020

vegaro Oct 7, 2020

aboedo Oct 8, 2020

vegaro Oct 7, 2020

aboedo Oct 8, 2020

vegaro Oct 8, 2020

vegaro Oct 7, 2020

aboedo Oct 8, 2020

vegaro Oct 7, 2020

aboedo Oct 8, 2020

aboedo Oct 8, 2020

vegaro Oct 9, 2020

		[self.operationDispatcher dispatchOnWorkerThread:^{
		[self.operationDispatcher dispatchOnWorkerThreadWithRandomDelay:NO :^{

		[self.deviceCache setOfferingsCacheTimestampToNow];
		__weak typeof(self) weakSelf = self;

		#import "RCLogUtils.h"
		#import "NSDictionary+RCExtensions.h"

Add jitter and extra cache for background processes #366

Add jitter and extra cache for background processes #366

Conversation

aboedo commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo Oct 5, 2020 •

edited