Fix: `Onyx.cleanCache` #81

kidroca · 2021-06-09T15:03:52Z

Details

Renamed Cache.remove to Cache.drop this method should not remove the key from getAllKeys's result
Updated Onyx.remove and Onyx.clear they set the cached value to null this way
the next connection does not have to wait to read null from storage

Related Issues

#76 (comment)
Expensify/App#3482

Automated Tests

Updated the tests for cache.drop

Linked PRs

`OnyxCache.remove` was renamed to `drop` and updated so that it does not remove the `key` but only the value This way the result of `getAllKeys` is unaffected Related to Expensify#76

When we remove values from storage we cache that the value was removed from storage The next time someone requests this value we can tell them right away it's null and they don't need to wait to read this from storage

kidroca · 2021-06-09T15:11:18Z

You might be concerned that cache will never remove values from getAllKeys and so when cache fills up
this short-circuit to null would not work:

react-native-onyx/lib/Onyx.js

Lines 373 to 382 in 9b03d95

    
           getAllKeys() 
        
               .then((keys) => { 
        
                   // Find all the keys matched by the config key 
        
                   const matchingKeys = _.filter(keys, key => isKeyMatch(mapping.key, key)); 
        
                   // If the key being connected to does not exist, initialize the value with null 
        
                   if (matchingKeys.length === 0) { 
        
                       sendDataToConnection(mapping, null); 
        
                       return; 
        
                   }

I don't think it's a problem as null will be returned from cache almost instantly here:

react-native-onyx/lib/Onyx.js

Lines 389 to 399 in 9b03d95

    
           if (mapping.withOnyxInstance && isCollectionKey(mapping.key)) { 
        
               Promise.all(_.map(matchingKeys, key => get(key))) 
        
                   .then(values => _.reduce(values, (finalObject, value, i) => ({ 
        
                       ...finalObject, 
        
                       [matchingKeys[i]]: value, 
        
                   }), {})) 
        
                   .then(val => sendDataToConnection(mapping, val)); 
        
           } else { 
        
               _.each(matchingKeys, (key) => { 
        
                   get(key).then(val => sendDataToConnection(mapping, val, key)); 
        
               });

get(key) would return null due to these changes: b0cf26c

An alternative is to add another OnyxCache method that actually removes keys from cache
or a 2nd parameter to Cache.drop - alsoDropKey - which would drop the key from getAllKeys
But both seem confusing to me - having 2 similar methods - drop and remove, or a 2nd optional parameter - cache.drop(key, true) and cache.drop(key)

Julesssss

This tests well and the regression is solved, thanks for the quick fix!

marcaaron

Left a couple of comments. Overall I agree that this is a bug:

on disconnect it tells cache to remove the iou from cache - because there are no more usages on screen
but cache.remove would also remove it from the storageKeys (the bug)

But less sure why we are calling cache.set('someKey', null) in the two places we are.

marcaaron · 2021-06-09T16:48:02Z

lib/Onyx.js

-    // Remove from cache
-    cache.remove(key);
+    // Cache the fact that the value was removed
+    cache.set(key, null);


Unsure why we need to do this. 🤔

marcaaron · 2021-06-09T16:48:42Z

lib/Onyx.js

@@ -639,7 +639,7 @@ function clear() {
        .then((keys) => {
            _.each(keys, (key) => {
                keyChanged(key, null);
-                cache.remove(key);
+                cache.set(key, null);


marcaaron

Gonna merge this so we can unbreak the production release, but still curious about the cache.set() stuff. I don't think anything bad can happen from it just not sure I understand it.

kidroca · 2021-06-09T17:10:05Z

@marcaaron

But less sure why we are calling cache.set('someKey', null) in the two places we are.

When we remove something using Onyx.remove we cache the fact that storage is now empty for that key - null
If we remove the value from cache the next time the value is retrieved it will have to be read from disk
only to find out that it's null
But if we set the value as null in cache we can tell the subscriber we have a cached value for this key and it is null

You can also verify this is the way AsyncStorage works by default for a missing key

await AsyncStorage.removeItem('myItem');
const value = await AsyncStorage.getItem('myItem'); // `null`

We're skipping the call to AsyncStorage.getItem when we already know the value would be null

marcaaron · 2021-06-09T17:19:52Z

we cache the fact that storage is now empty for that key - null
If we remove the value from cache the next time the value is retrieved it will have to be read from disk
only to find out that it's null

Cool thanks, I get that part, I just don't see how it is related to the linked issue or regression and someone looking at the code might not understand the purpose. Reaching into storage is how we treat most other "gets" for a key that does not exist in Onyx so it's not clear why we are making an exception here or whether it makes any difference.

marcaaron · 2021-06-09T17:20:56Z

Cache the fact that the value was removed

comment could be improved as it doesn't explain why we are doing this

kidroca · 2021-06-09T17:56:36Z

Reaching into storage is how we treat most other "gets" for a key that does not exist in Onyx so it's not clear why we are making an exception here or whether it makes any difference.

Well now instead of reaching for storage we know ahead of time that the key is not there - when something deliberately removed a key using remove or all values using clear

Previously (actually still is for some cases) this was achieved by using getAllKeys and seeing that the key is not there, now we no longer have to do it if it's solely for checking if there's a value (in connect it's not solely for that but to also get collection keys)

As you can see we no longer remove keys for the result of getAllKeys. If we do nothing the code here might continue and read from storage only to find out the underlying value is null

react-native-onyx/lib/Onyx.js

Lines 373 to 382 in 9b03d95

    
           getAllKeys() 
        
               .then((keys) => { 
        
                   // Find all the keys matched by the config key 
        
                   const matchingKeys = _.filter(keys, key => isKeyMatch(mapping.key, key)); 
        
                   // If the key being connected to does not exist, initialize the value with null 
        
                   if (matchingKeys.length === 0) { 
        
                       sendDataToConnection(mapping, null); 
        
                       return; 
        
                   }

I've tried to explain this here: #81 (comment)

If you want to also remove the key from the result of getAllKeys, we'll have to have a separate method or add a 2nd parameter to cache.drop

Just like we can cache the fact that we have something, we can do the same to cache that we know there's no value in storage for a given key - and skip the call to storage

marcaaron · 2021-06-09T18:08:26Z

I'll try being more direct here:

The PR introduced a change that is not related to the issue it is fixing
The code presented has inadequate documentation

Can you elaborate (in the code) so that others (not me) in the future can understand what your intentions were?

kidroca · 2021-06-09T18:38:33Z

The PR introduced a change that is not related to the issue it is fixing

Every change here is related to the issue

logic in cache.remove was updated
some usages that used cache.remove were updated to use cache.set(key, null), because cache.remove no longer removes the key from getAllKeys

Can you elaborate (in the code) so that others (not me) in the future can understand what your intentions were?

My intentions are explained here and in the git log. The code for remove and clear explains itself

I don't think people reading the code in the feature would be confused that remove(key) is doing cache.set(key, null) - it's pretty clear what that means

If you need me to do something specific, add different comments or change the code like I've suggested as alternative I can do it tomorrow, but otherwise I don't know see what can I do

TomatoToaster · 2021-06-10T06:18:37Z

~~I think a change here is causing the regression. More details here: Expensify/App#3485 (comment)~~

~~I think I have an idea for a fix, testing it out right now.~~

kidroca · 2021-06-10T06:45:48Z

Are you sure?

The issue you're talking about was discovered before this PR was even merged - "iOS - App crashes after turning off the internet connection" - the code you're quoting wasn't included in the app at that time

And the update that bumps E.cash to use the Onyx changes made here happened even later that day: Expensify/App#3483

Check the timing of this comment and the time of the merges - https://expensify.slack.com/archives/C01GTK53T8Q/p1623256929142800

TomatoToaster · 2021-06-10T07:28:56Z

Ah yes sorry you are 100% right. I've got the wrong PR.

More grandly, it's a change from this diff between the versions of react-native-onyx as seen here: https://github.com/Expensify/react-native-onyx/compare/d0a5d12c08f2f029e3038fb3516a93a9403e9746..52c254fbc4b96224d2ac087e57ef6efc4210ccc1

(This is comparing the production react-native-onyx in production with what's on staging/master).

This just happened to be the most recent onyx change so I thought it was from here, but it's a little tricky to pinpoint the exact PR.

I'll put more details in the Cash issue I got this from.

kidroca added 2 commits June 9, 2021 17:52

fix: disconnect affects the cached value for getAllKeys

d322991

`OnyxCache.remove` was renamed to `drop` and updated so that it does not remove the `key` but only the value This way the result of `getAllKeys` is unaffected Related to Expensify#76

refactor: update Onyx.remove and Onyx.clear to cache a null value

b0cf26c

When we remove values from storage we cache that the value was removed from storage The next time someone requests this value we can tell them right away it's null and they don't need to wait to read this from storage

kidroca requested a review from a team as a code owner June 9, 2021 15:03

MelvinBot requested review from joelbettner and removed request for a team June 9, 2021 15:04

kidroca mentioned this pull request Jun 9, 2021

Feature: Onyx Cache #76

Merged

Julesssss approved these changes Jun 9, 2021

View reviewed changes

kidroca mentioned this pull request Jun 9, 2021

IOU - Currencies are not displayed after clicking the currency symbol for a second time Expensify/App#3482

Closed

marcaaron requested changes Jun 9, 2021

View reviewed changes

marcaaron self-requested a review June 9, 2021 17:02

marcaaron approved these changes Jun 9, 2021

View reviewed changes

marcaaron merged commit 52c254f into Expensify:master Jun 9, 2021

kidroca deleted the kidroca/onyx-cleanCache-fix branch June 9, 2021 17:11

Julesssss mentioned this pull request Jun 9, 2021

[NO-QA] Bump Onyx commit to fix IOU regression Expensify/App#3483

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: `Onyx.cleanCache` #81

Fix: `Onyx.cleanCache` #81

kidroca commented Jun 9, 2021 •

edited

Loading

kidroca commented Jun 9, 2021 •

edited

Loading

Julesssss left a comment

marcaaron left a comment

marcaaron Jun 9, 2021

marcaaron Jun 9, 2021

marcaaron left a comment

kidroca commented Jun 9, 2021 •

edited

Loading

marcaaron commented Jun 9, 2021

marcaaron commented Jun 9, 2021

kidroca commented Jun 9, 2021

marcaaron commented Jun 9, 2021

kidroca commented Jun 9, 2021

TomatoToaster commented Jun 10, 2021 •

edited

Loading

kidroca commented Jun 10, 2021 •

edited

Loading

TomatoToaster commented Jun 10, 2021

Fix: Onyx.cleanCache #81

Fix: Onyx.cleanCache #81

Conversation

kidroca commented Jun 9, 2021 • edited Loading

Details

Related Issues

Automated Tests

Linked PRs

kidroca commented Jun 9, 2021 • edited Loading

Julesssss left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Jun 9, 2021

Choose a reason for hiding this comment

marcaaron Jun 9, 2021

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

kidroca commented Jun 9, 2021 • edited Loading

marcaaron commented Jun 9, 2021

marcaaron commented Jun 9, 2021

kidroca commented Jun 9, 2021

marcaaron commented Jun 9, 2021

kidroca commented Jun 9, 2021

TomatoToaster commented Jun 10, 2021 • edited Loading

kidroca commented Jun 10, 2021 • edited Loading

TomatoToaster commented Jun 10, 2021

Fix: `Onyx.cleanCache` #81

Fix: `Onyx.cleanCache` #81

kidroca commented Jun 9, 2021 •

edited

Loading

kidroca commented Jun 9, 2021 •

edited

Loading

kidroca commented Jun 9, 2021 •

edited

Loading

TomatoToaster commented Jun 10, 2021 •

edited

Loading

kidroca commented Jun 10, 2021 •

edited

Loading