Fix race condition with clearing Onyx and the SyncQueue

lib/Onyx.js

@@ -1050,6 +1050,13 @@ 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 = _.reduce(keysToPreserve, (finalArray, keyToPreserve) => {
+                finalArray.push([keyToPreserve, cache.getValue(keyToPreserve)]);
+                return finalArray;
+            }, []);
+
             // 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 +1109,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.concat(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,11 @@ 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
+        // 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 = () => Storage.getAllKeys()
-            .then(keys => _.map(keys, key => this.removeItem(key)))
-            .then(tasks => Promise.all(tasks));
+            .then(allKeys => _.each(allKeys, raiseStorageSyncEvent))
+            .then(() => Storage.clear());
 
         // 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

@@ -104,4 +104,23 @@ describe('storage/providers/LocalForage', () => {
                     });
             });
     });
+
+    it('clear', () => {
+        // Use fake timers, so we can manipulate time at our will for this test.
+        jest.useFakeTimers();
+
+        // Given an implementation of setItem that resolves after 1000ms
+        localforage.setItem = jest.fn(() => new Promise(resolve => setTimeout(resolve, 1000)));
+
+        // When we call setItem 5 times, but then call clear after only 1000ms
+        for (let i = 0; i < 5; i++) {
+            StorageProvider.setItem(`key${i}`, `value${i}`);
+        }
+        jest.advanceTimersByTime(1000);
+        StorageProvider.clear();
+        jest.advanceTimersByTime(4000);
+
+        // Then setItem should only have been called once since all other calls were aborted when we called clear()
+        expect(localforage.setItem).toHaveBeenCalledTimes(1);
+    });
 });