Fix issue #15321: The app crashes when the user is logged into multiple tabs and logs out of one of the tabs by tienifr · Pull Request #237 · Expensify/react-native-onyx

tienifr · 2023-02-28T10:42:57Z

Details

This PR changes the logic of the clear() function which cause issue Expensify/App#15321 so that instead of removing all the keys then adding the default value for some important keys back, it only removes the keys that are not needed and retains the important keys.
It also address the performance concern of Expensify/App#15321 (comment) since the key removals are execute asynchronously. The total execution time has almost no difference compared to the old implementation.

Fixed Issues

$ Expensify/App#15321
$ Expensify/App#15321 (comment)

Tests

Open the app in the browser
Open another tab in the same browser
Log out from one of the tabs
Verify that in other tab the app is not crashed and logged out to the login page.

QA Steps

Open the app in the browser
Open another tab in the same browser
Log out from one of the tabs
Verify that in other tab the app is not crashed and logged out to the login page.

Screenshots

Web

new_web.mp4

crash_web_safari.mov

Mobile Web - Chrome

new_mweb_chrome.mp4

Mobile Web - Safari

crash_mweb_safari.mov

Desktop

crash_desktop.mov

iOS

crash_ios.mov

Android

new_android.mp4

github-actions · 2023-02-28T10:43:14Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

tienifr · 2023-02-28T10:44:08Z

I have read the CLA Document and I hereby sign the CLA

madmax330 · 2023-02-28T15:39:04Z

lib/Onyx.js


-            // Call clear() and make sure that the default key/values and the key/values from the parameter
-            // are preserved in storage. This makes sure to always leave storage in a state that contains
-            // all the default values and any additional values that we want to remain after the database is cleared.


Let's keep a comment here to explain what we're doing and why

@madmax330 Thanks for pointing that out. I have updated, please help to check again

updated it to only say what and not why...

roryabraham

I think this function could use some additional cleanup. I found it pretty hard to follow, and for the most part we just need one main loop.

function clear(keysToPreserve = []) {
    return getAllKeys()
        .then((keys) => {
            const keysToBeClearedFromStorage = [];
            const keyValuesToResetAsCollection = {};
            const keyValuesToResetIndividually = {};

            // The only keys that should not be cleared are:
            // 1. Anything specifically passed in keysToPreserve (because some keys like language preferences, offline
            //      status, or activeClients need to remain in Onyx even when signed out)
            // 2. Any keys with a default state (because they need to remain in Onyx as their default, and setting them
            //      to null would cause unknown behavior)
            _.each(keys, (key) => {
                const isKeyToPreserve = _.contains(keysToPreserve, key);
                const isDefaultKey = _.has(defaultKeyStates, key);

                // If the key is being removed or reset to default:
                // 1. Update it in the cache
                // 2. Figure out whether it is a collection key or not,
                //      since collection key subscribers need to be updated differently
                if (!isKeyToPreserve) {
                    const newValue = defaultKeyStates[key] || null;
                    cache.set(key, newValue);
                    const collectionKey = key.substring(0, key.indexOf('_') + 1);
                    if (collectionKey) {
                        if (!keyValuesToResetAsCollection[collectionKey]) {
                            keyValuesToResetAsCollection[collectionKey] = {};
                        }
                        keyValuesToResetAsCollection[collectionKey][key] = newValue;
                    } else {
                        keyValuesToResetIndividually[key] = newValue;
                    }
                }

                if (isKeyToPreserve || isDefaultKey) {
                    return;
                }

                // If it isn't preserved and doesn't have a default, we'll remove it
                keysToBeClearedFromStorage.push(key);
            });

            // Notify the subscribers for each key/value group so they can receive the new values
            _.each(keyValuesToResetIndividually, (value, key) => {
                notifySubscribersOnNextTick(key, value);
            });
            _.each(keyValuesToResetAsCollection, (value, key) => {
                notifyCollectionSubscribersOnNextTick(key, value);
            });

            // Remove only the items that we want cleared from storage
            return Storage.removeItems(keysToBeClearedFromStorage);
        });

lib/Onyx.js

sobitneupane

@tienifr I am not getting the excepted results. Can you please rerun the tests with latest changes?

Screen.Recording.2023-03-01.at.18.47.46.mov

lib/storage/WebStorage.js

lib/storage/providers/LocalForage.js

tienifr · 2023-03-01T17:11:15Z

@roryabraham Thanks for your comment, but the PR isn't completed, I still need to change a few things to make it work

tienifr · 2023-03-01T17:27:05Z

@roryabraham @sobitneupane I just fixed all comments, also tested again and it works correctly. Please help to check again. Thanks

Screen.Recording.2023-03-02.at.00.25.02.mov

lib/Onyx.js

tienifr · 2023-03-02T03:34:22Z

@roryabraham I agree with you that we should only mutilSet the default keys. I just fixed that, please check again.

sobitneupane

I am getting following console errors.

Why does the other tab updates only when the tab is active? You can notice in the video that tab updates only when the user switches to the tab. It indicates that the onyx is synced only when the tab is active which can create some serious issues.

Screen.Recording.2023-03-02.at.18.55.53.mov

roryabraham · 2023-03-03T01:12:19Z

lib/Onyx.js

-            // all the default values and any additional values that we want to remain after the database is cleared.
-            return Storage.clear()
-                .then(() => Storage.multiSet([...defaultKeyValuePairs, ...keyValuesToPreserve]));
+            const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));


Unnecessary spread:

Suggested change

const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));

const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, keysToPreserve));

Fixed, thanks

roryabraham · 2023-03-03T01:26:57Z

I am getting following console errors.

@sobitneupane Did you remember to run npm install localforage-removeitems before copying over this change and running E/App? I saw the same errors and was able to fix them by installing the package

roryabraham · 2023-03-03T01:27:36Z

package.json

  "dependencies": {
    "ascii-table": "0.0.9",
    "fast-equals": "^4.0.3",
+    "localforage-removeitems": "^1.4.0",


I think this should be a peerDependency instead of a dependency

Fixed, thanks

package.json

tienifr · 2023-03-03T18:17:32Z

@roryabraham I've fixed your comments, pls help review again
Thanks!

tienifr · 2023-03-03T18:54:05Z

Why does the other tab updates only when the tab is active? You can notice in the video that tab updates only when the user switches to the tab. It indicates that the onyx is synced only when the tab is active which can create some serious issues.

@sobitneupane I think this is not related to the scope of this PR (or this issue), since even if we apply only this patch on App without this react-native-onyx root cause fix, we can still see that delay behavior.

roryabraham

This PR is looking good to me 👍🏼

tienifr · 2023-03-06T17:46:27Z

lib/storage/providers/AsyncStorage.js

+     * @param {Array} keys
+     * @returns {Promise}
+     */
+    removeItems: keys => Promise.all(_.map(keys, key => AsyncStorage.removeItem(key))),


This is required because we're using AsyncStorage as the mock provider for NativeStorage in unit tests, so removeItems needs to be defined here as well
cc @roryabraham @sobitneupane

tienifr · 2023-03-08T15:54:06Z

@roryabraham, I've added __mocks__/localforage-removeitems.js file to mock removeItems function on test environment. I have also updated the logic in clear function to add defaultKeys to keys like the old logic. Please help take a look, thanks.

sobitneupane · 2023-03-09T17:17:51Z

@sobitneupane I think this is not related to the scope of this PR (or this issue), since even if we apply only this patch on App without this react-native-onyx root cause fix, we can still see that delay behavior.

I don't agree with you @tienifr. I tested the PR. This is the result in the PR.

Screen.Recording.2023-03-09.at.22.41.30.mov

And following is the result with the changes in this PR.

Screen.Recording.2023-03-09.at.22.59.21.mov

Please notice the unread indicators in both the videos. With the change in this PR, unread indicator only goes away after user switches to the tab indicating onyx is cleared only after user switches to the tab.

tienifr · 2023-03-10T11:00:04Z

@sobitneupane Thanks for pointing that out. I fixed this issue and here is the result. Please help check again.

Screen.Recording.2023-03-10.at.17.54.56.mp4

sobitneupane · 2023-03-10T17:30:05Z

@tienifr Many changes are made since PR is created. So, can you please update the screen recordings in PR body. Also, please do include screen recordings for heavy traffic accounts.

tienifr · 2023-03-11T08:30:42Z

@tienifr Many changes are made since PR is created. So, can you please update the screen recordings in PR body. Also, please do include screen recordings for heavy traffic accounts.

@sobitneupane

Thanks for the heads up! I have updated the screen recordings with HTA. I also made some small changes so the code could pass the lint test. Please have a look.

roryabraham · 2023-03-11T14:40:48Z

lib/Onyx.js

+            const keysToBeClearedFromStorage = [];
+            const keyValuesToResetAsCollection = {};
+            const keyValuesToResetIndividually = {};
+            const defaultKeys = _.keys(_.omit(defaultKeyStates, ...keysToPreserve));


Why do we need the keysToReset here? It seems to me like this logic should be covered in the main loop right?

Also, if this is needed for some reason I'm not seeing, there's an unnecessary spread:

Suggested change

const defaultKeys = _.keys(_.omit(defaultKeyStates, ...keysToPreserve));

const defaultKeys = _.keys(_.omit(defaultKeyStates, keysToPreserve));

@roryabraham Thanks for your comment. I think we should notify subscribers when data has been cleared as I can see this test on onyxTest.js. After checking the old logic I see we push the defaultKeyValuePairs to keyValuesToReset, that why I decide to have keysToReset. You can check the test

react-native-onyx/tests/unit/onyxTest.js

Line 95 in 4887af4

it('should notify subscribers when data has been cleared', () => {

the keys = [ 'test' ] and defaultKeyStates = { otherTest: 42 }, if we don't push otherTest to keys, we'll miss this value.

And yes, you're right, having spread is unnecessary. I updated my PR to remove the spread.

react-native-onyx/lib/Onyx.js

Lines 1070 to 1072 in 4887af4

// Add the default key value pairs to the keyValuesToReset so that they get set back to their default values

// when we clear Onyx

keyValuesToReset.push(...defaultKeyValuePairs);

Thanks for explaining. I've spent some time testing, and I still don't think this code is needed. I modified the onyxTest a bit to test that default keys are reset:

diff --git a/tests/unit/onyxTest.js b/tests/unit/onyxTest.js index 50b8608..e9a955e 100644 --- a/tests/unit/onyxTest.js +++ b/tests/unit/onyxTest.js @@ -112,9 +112,10 @@ describe('Onyx', () => { return waitForPromisesToResolve() .then(() => Onyx.set(ONYX_KEYS.TEST_KEY, 'test')) + .then(() => Onyx.set(ONYX_KEYS.OTHER_TEST, 420)) .then(() => { expect(testKeyValue).toBe('test'); - expect(otherTestValue).toBe(42); + expect(otherTestValue).toBe(420); return Onyx.clear().then(waitForPromisesToResolve); }) .then(() => {

Then I reset the code to how it was before:

diff --git a/lib/Onyx.js b/lib/Onyx.js index 910b759..2667754 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -1050,16 +1050,13 @@ function clear(keysToPreserve = []) { const keysToBeClearedFromStorage = []; const keyValuesToResetAsCollection = {}; const keyValuesToResetIndividually = {}; - const defaultKeys = _.keys(_.omit(defaultKeyStates, keysToPreserve)); - const keysToReset = _.difference(keys, defaultKeys); - keysToReset.push(...defaultKeys); // The only keys that should not be cleared are: // 1. Anything specifically passed in keysToPreserve (because some keys like language preferences, offline // status, or activeClients need to remain in Onyx even when signed out) // 2. Any keys with a default state (because they need to remain in Onyx as their default, and setting them // to null would cause unknown behavior) - _.each(keysToReset, (key) => { + _.each(keys, (key) => { const isKeyToPreserve = _.contains(keysToPreserve, key); const isDefaultKey = _.has(defaultKeyStates, key);

And everything worked as expected. Since this is simpler, let's change it back.

sobitneupane

Screenshots/Videos

Web

Screen.Recording.2023-03-13.at.18.25.05.mov

Mobile Web - Chrome

Screen.Recording.2023-03-13.at.18.29.38.mov

Mobile Web - Safari

Screen.Recording.2023-03-13.at.18.29.38.mov

Desktop

Screen.Recording.2023-03-14.at.16.25.04.mov

iOS

Screen.Recording.2023-03-13.at.18.32.15.mov

Android

Screen.Recording.2023-03-13.at.18.33.43.mov

roryabraham

Also, not going to require this since it's probably fine if we don't, but I think it might be good to prevent updating subscribers unless the data actually changes:

 
@@ -1068,16 +1065,19 @@ function clear(keysToPreserve = []) {
                 // 2. Figure out whether it is a collection key or not,
                 //      since collection key subscribers need to be updated differently
                 if (!isKeyToPreserve) {
+                    const oldValue = cache.getValue(key);
                     const newValue = _.get(defaultKeyStates, key, null);
-                    cache.set(key, newValue);
-                    const collectionKey = key.substring(0, key.indexOf('_') + 1);
-                    if (collectionKey) {
-                        if (!keyValuesToResetAsCollection[collectionKey]) {
-                            keyValuesToResetAsCollection[collectionKey] = {};
+                    if (newValue !== oldValue) {
+                        cache.set(key, newValue);
+                        const collectionKey = key.substring(0, key.indexOf('_') + 1);
+                        if (collectionKey) {
+                            if (!keyValuesToResetAsCollection[collectionKey]) {
+                                keyValuesToResetAsCollection[collectionKey] = {};
+                            }
+                            keyValuesToResetAsCollection[collectionKey][key] = newValue;
+                        } else {
+                            keyValuesToResetIndividually[key] = newValue;
                         }
-                        keyValuesToResetAsCollection[collectionKey][key] = newValue;
-                    } else {
-                        keyValuesToResetIndividually[key] = newValue;
                     }
                 }

tienifr · 2023-03-13T17:59:14Z

@roryabraham Yes, you're right. So I updated my PR as your suggestion, I also removed unnecessary test case. Please help review again. Thanks.

@sobitneupane Here is my result on desktop

Screen.Recording.2023-03-14.at.00.57.34.mov

sobitneupane · 2023-03-15T16:13:36Z

@roryabraham I think we are good to merge this PR.

tienifr · 2023-03-21T02:14:17Z

@roryabraham

Are we good to go for this PR to be merged?

tienifr · 2023-03-22T03:00:50Z

@roryabraham Could you check again and merged it if there are no more problems?

roryabraham · 2023-03-22T19:47:50Z

Shoot, was just about to merge this, but I can't because it contains unsigned commits. @tienifr Please either retroactively sign all these commits then force-push to this branch, or create a new PR with 100% signed commits and tag us again.

tienifr · 2023-03-23T03:30:05Z

@roryabraham My bad, I just squashed commit and forced push. Please check again and merge this PR

marcaaron · 2023-07-14T20:39:46Z

__mocks__/localforage-removeitems.js

+    const newLocalforage = localforage;
+    newLocalforage.removeItems = keys => new Promise((resolve) => {
+        _.each(keys, (key) => {
+            delete newLocalforage.storageMap[key];


I'm confused by this. Does this actually delete the stuff from IndexedDB? It looks like it would not...

Ohhh nvm it's a mock...

Well this is a mock. This code has changed a bit since this PR was created, but the mock isn't actually writing anything to IndexedDB. storageMapInternal is just an object so I think delete does work here?

tienifr requested a review from a team as a code owner February 28, 2023 10:42

melvin-bot bot requested review from madmax330 and removed request for a team February 28, 2023 10:43

tienifr mentioned this pull request Feb 28, 2023

[HOLD #34289] [$2000] The app crashes when the user is logged into multiple tabs and logs out of one of the tabs Expensify/App#15321

Closed

6 tasks

madmax330 requested a review from roryabraham February 28, 2023 15:37

madmax330 reviewed Feb 28, 2023

View reviewed changes

roryabraham requested changes Feb 28, 2023

View reviewed changes

lib/Onyx.js Outdated Show resolved Hide resolved

lib/Onyx.js Outdated Show resolved Hide resolved

sobitneupane reviewed Mar 1, 2023

View reviewed changes

roryabraham requested changes Mar 1, 2023

View reviewed changes

lib/storage/WebStorage.js Outdated Show resolved Hide resolved

lib/storage/providers/LocalForage.js Outdated Show resolved Hide resolved

roryabraham requested changes Mar 1, 2023

View reviewed changes

lib/storage/providers/LocalForage.js Outdated Show resolved Hide resolved

roryabraham requested changes Mar 1, 2023

View reviewed changes

lib/Onyx.js Outdated Show resolved Hide resolved

roryabraham requested changes Mar 1, 2023

View reviewed changes

lib/Onyx.js Outdated Show resolved Hide resolved

sobitneupane reviewed Mar 2, 2023

View reviewed changes

roryabraham requested changes Mar 3, 2023

View reviewed changes

tienifr commented Mar 3, 2023

View reviewed changes

package.json Show resolved Hide resolved

roryabraham previously approved these changes Mar 3, 2023

View reviewed changes

tienifr dismissed roryabraham’s stale review via f71fa5b March 6, 2023 17:43

tienifr commented Mar 6, 2023

View reviewed changes

tienifr force-pushed the fix/15321 branch from b935d38 to 343df67 Compare March 8, 2023 12:10

roryabraham reviewed Mar 11, 2023

View reviewed changes

sobitneupane reviewed Mar 13, 2023

View reviewed changes

roryabraham reviewed Mar 13, 2023

View reviewed changes

sobitneupane previously approved these changes Mar 14, 2023

View reviewed changes

roryabraham previously approved these changes Mar 22, 2023

View reviewed changes

tienifr dismissed stale reviews from roryabraham and sobitneupane via d9c4aeb March 23, 2023 03:08

tienifr force-pushed the fix/15321 branch from cefaaca to d9c4aeb Compare March 23, 2023 03:08

fix: modify clear method and add removeitems function

ba2c1e5

tienifr force-pushed the fix/15321 branch from d9c4aeb to ba2c1e5 Compare March 23, 2023 03:36

roryabraham approved these changes Mar 23, 2023

View reviewed changes

roryabraham merged commit b040c0d into Expensify:main Mar 23, 2023

roryabraham mentioned this pull request Apr 24, 2023

Clear cache in Onyx.clear() #245

Merged

marcaaron reviewed Jul 14, 2023

View reviewed changes

This was referenced Jul 14, 2023

Revert back to Storage.clear() in Onyx.clear() instead of removeItems(). Abort the setItem() queue. #279

Closed

Setting a lot of data in Onyx is slow Expensify/App#21766

Closed

	const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));
	const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, keysToPreserve));

	const defaultKeys = _.keys(_.omit(defaultKeyStates, ...keysToPreserve));
	const defaultKeys = _.keys(_.omit(defaultKeyStates, keysToPreserve));

	// Add the default key value pairs to the keyValuesToReset so that they get set back to their default values
	// when we clear Onyx
	keyValuesToReset.push(...defaultKeyValuePairs);

Conversation

tienifr commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

github-actions bot commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tienifr commented Feb 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sobitneupane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tienifr commented Mar 1, 2023

Uh oh!

tienifr commented Mar 1, 2023

Uh oh!

Uh oh!

Uh oh!

tienifr commented Mar 2, 2023

Uh oh!

sobitneupane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tienifr commented Mar 3, 2023

Uh oh!

tienifr commented Mar 3, 2023

Uh oh!

roryabraham left a comment

Choose a reason for hiding this comment

Uh oh!

tienifr Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tienifr commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobitneupane commented Mar 9, 2023

Uh oh!

tienifr commented Mar 10, 2023

Uh oh!

sobitneupane commented Mar 10, 2023

Uh oh!

tienifr commented Feb 28, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023 •

edited

Loading

roryabraham commented Mar 3, 2023 •

edited

Loading

tienifr Mar 6, 2023 •

edited

Loading

tienifr commented Mar 8, 2023 •

edited

Loading

tienifr commented Mar 11, 2023 •

edited

Loading

sobitneupane left a comment •

edited

Loading