Merge pull request #220 from Expensify/tgolen-implement-multiremove2

Fix race condition with clearing Onyx and the SyncQueue

lib/Onyx.js

@@ -1050,6 +1050,10 @@ function clear(keysToPreserve = []) {
             const keyValuesToReset = [];
             const defaultKeys = _.keys(defaultKeyStates);
 
+            // Get all the values for the keys that need to be preserved. These key/value pairs will be set
+            // in Onyx after the database is cleared().
+            const keyValuesToPreserve = _.map(keysToPreserve, key => [key, cache.getValue(key)]);
+
             // 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)
@@ -1102,7 +1106,11 @@ function clear(keysToPreserve = []) {
                 notifyCollectionSubscribersOnNextTick(key, value);
             });
 
-            return Storage.multiSet(keyValuesToReset);
+            // 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.
+            return Storage.clear()
+                .then(() => Storage.multiSet([...defaultKeyValuePairs, ...keyValuesToPreserve]));
         });
 }
 

lib/SyncQueue.js

@@ -21,6 +21,14 @@ export default class SyncQueue {
         this.run = run;
     }
 
+    /**
+     * Stop the queue from being processed and clear out any existing tasks
+     */
+    abort() {
+        this.queue = [];
+        this.isProcessing = false;
+    }
+
     process() {
         if (this.isProcessing || this.queue.length === 0) {
             return;

lib/storage/WebStorage.js

@@ -1,3 +1,8 @@
+/**
+ * This file is here to wrap LocalForage with a layer that provides data-changed events like the ones that exist
+ * when using LocalStorage APIs in the browser. These events are great because multiple tabs can listen for when
+ * data changes and then stay up-to-date with everything happening in Onyx.
+ */
 import _ from 'underscore';
 import Storage from './providers/LocalForage';
 
@@ -27,10 +32,23 @@ const webStorage = {
             .then(() => raiseStorageSyncEvent(key));
 
         // If we just call Storage.clear other tabs will have no idea which keys were available previously
-        // so that they can call keysChanged for them. That's why we iterate and remove keys one by one
-        this.clear = () => Storage.getAllKeys()
-            .then(keys => _.map(keys, key => this.removeItem(key)))
-            .then(tasks => Promise.all(tasks));
+        // so that they can call keysChanged for them. That's why we iterate over every key and raise a storage sync
+        // event for each one
+        this.clear = () => {
+            let allKeys;
+
+            // They keys must be retreived before storage is cleared or else the list of keys would be empty
+            return Storage.getAllKeys()
+                .then((keys) => {
+                    allKeys = keys;
+                })
+                .then(() => Storage.clear())
+                .then(() => {
+                    // Now that storage is cleared, the storage sync event can happen which is a more atomic action
+                    // for other browser tabs
+                    _.each(allKeys, raiseStorageSyncEvent);
+                });
+        };
 
         // This listener will only be triggered by events coming from other tabs
         global.addEventListener('storage', (event) => {

lib/storage/providers/AsyncStorage.js

@@ -74,7 +74,7 @@ const provider = {
     removeItem: AsyncStorage.removeItem,
 
     /**
-     * Clear absolutely everything from storage
+     * Clear everything from storage
      * @returns {Promise<void>}
      */
     clear: AsyncStorage.clear,

lib/storage/providers/LocalForage.js

@@ -75,10 +75,13 @@ const provider = {
     },
 
     /**
-     * Clear absolutely everything from storage
+     * Clear everything from storage and also stops the SyncQueue from adding anything more to storage
      * @returns {Promise<void>}
      */
-    clear: localforage.clear,
+    clear() {
+        this.setItemQueue.abort();
+        return localforage.clear();
+    },
 
     /**
      * Returns all keys available in storage

tests/unit/onyxClearNativeStorageTest.js

@@ -43,28 +43,6 @@ describe('Set data while storage is clearing', () => {
         return Onyx.clear();
     });
 
-    it('should persist the value of Onyx.merge when called between the cache and storage clearing', () => {
-        expect.assertions(3);
-
-        // Given that Onyx is completely clear
-        // When Onyx.clear() is called
-        Onyx.clear();
-
-        // When merge is called between the cache and storage clearing, on a key with a default key state
-        Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE);
-        return waitForPromisesToResolve()
-            .then(() => {
-                // Then the value in Onyx, the cache, and the storage is the merged value
-                expect(onyxValue).toBe(MERGED_VALUE);
-                const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY);
-                expect(cachedValue).toBe(MERGED_VALUE);
-
-                // Use parseInt to convert the string value from the storage mock back to an integer
-                return Storage.getItem(ONYX_KEYS.DEFAULT_KEY)
-                    .then(storedValue => expect(parseInt(storedValue, 10)).toBe(MERGED_VALUE));
-            });
-    });
-
     it('should replace the value of Onyx.set with the default key state in the cache', () => {
         expect.assertions(3);
 
@@ -120,8 +98,7 @@ describe('Set data while storage is clearing', () => {
             .then(() => {
                 // Then the value of the preserved key is also still set in both the cache and storage
                 expect(regularKeyOnyxValue).toBe(SET_VALUE);
-                const regularKeyCachedValue = cache.getValue(ONYX_KEYS.REGULAR_KEY);
-                expect(regularKeyCachedValue).toBe(SET_VALUE);
+                expect(cache.getValue(ONYX_KEYS.REGULAR_KEY)).toBe(SET_VALUE);
                 return Storage.getItem(ONYX_KEYS.REGULAR_KEY)
                     .then(storedValue => expect(parseInt(storedValue, 10)).toBe(SET_VALUE));
             });

tests/unit/onyxClearWebStorageTest.js

@@ -50,26 +50,6 @@ describe('Set data while storage is clearing', () => {
         return Onyx.clear();
     });
 
-    it('should persist the value of Onyx.merge when called between the cache and storage clearing', () => {
-        expect.assertions(3);
-
-        // Given that Onyx is completely clear
-        // When Onyx.clear() is called
-        Onyx.clear();
-
-        // When merge is called between the cache and storage clearing, on a key with a default key state
-        Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE);
-        return waitForPromisesToResolve()
-            .then(() => {
-                // Then the value in Onyx, the cache, and the storage is the merged value
-                expect(onyxValue).toBe(MERGED_VALUE);
-                const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY);
-                expect(cachedValue).toBe(MERGED_VALUE);
-                const storedValue = Storage.getItem(ONYX_KEYS.DEFAULT_KEY);
-                return expect(storedValue).resolves.toBe(MERGED_VALUE);
-            });
-    });
-
     it('should replace the value of Onyx.set with the default key state in the cache', () => {
         expect.assertions(3);
 

tests/unit/onyxMultiMergeWebStorageTest.js

@@ -141,12 +141,8 @@ describe('Onyx.mergeCollection() amd WebStorage', () => {
     });
 
     it('setItem() and multiMerge()', () => {
-        // When Onyx is cleared, since it uses multiSet() to clear out all the values, the keys will remain with null for all the values
-        expect(localforageMock.storageMap).toEqual({
-            test_1: null,
-            test_2: null,
-            test_3: null,
-        });
+        // Onyx should be empty after clear() is called
+        expect(localforageMock.storageMap).toEqual({});
 
         // Given no previous data and several calls to setItem and call to mergeCollection to update a given key
 

tests/unit/storage/providers/LocalForageProviderTest.js

@@ -2,6 +2,8 @@ import localforage from 'localforage';
 import _ from 'underscore';
 
 import StorageProvider from '../../../../lib/storage/providers/LocalForage';
+import createDeferredTask from '../../../../lib/createDeferredTask';
+import waitForPromisesToResolve from '../../../utils/waitForPromisesToResolve';
 
 describe('storage/providers/LocalForage', () => {
     const SAMPLE_ITEMS = [
@@ -104,4 +106,35 @@ describe('storage/providers/LocalForage', () => {
                     });
             });
     });
+
+    it('clear', () => {
+        // We're creating a Promise which we programatically control when to resolve.
+        const task = createDeferredTask();
+
+        // We configure localforage.setItem to return this promise the first time it's called and to otherwise return resolved promises
+        localforage.setItem = jest.fn()
+            . mockReturnValue(Promise.resolve()) // Default behavior
+            . mockReturnValueOnce(task.promise); // First call behavior
+
+        // Make 5 StorageProvider.setItem calls - this adds 5 items to the queue and starts executing the first localForage.setItem
+        for (let i = 0; i < 5; i++) {
+            StorageProvider.setItem(`key${i}`, `value${i}`);
+        }
+
+        // At this point,`localForage.setItem` should have been called once, but we control when it resolves, and we'll keep it unresolved.
+        // This simulates the 1st localForage.setItem taking a random time.
+        // We then call StorageProvider.clear() while the first localForage.setItem isn't completed yet.
+        StorageProvider.clear();
+
+        // Any calls that follow this would have been queued - so we don't expect more than 1 `localForage.setItem` call after the
+        // first one resolves.
+        task.resolve();
+
+        // waitForPromisesToResolve() makes jest wait for any promises (even promises returned as the result of a promise) to resolve.
+        // If StorageProvider.clear() does not abort the queue, more localForage.setItem calls would be executed because they would
+        // be sitting in the setItemQueue
+        return waitForPromisesToResolve().then(() => {
+            expect(localforage.setItem).toHaveBeenCalledTimes(1);
+        });
+    });
 });