diff --git a/lib/Onyx.ts b/lib/Onyx.ts index fd1238388..6c79a8c26 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -78,7 +78,11 @@ function init({ // Capture the previous cached value before cache.set() so notifyCollection() can diff old vs new per member. const previousValue = isCollectionMember ? cache.get(key) : undefined; + // The key stays indexed because this answer can predate a write of ours still on its way to storage, and only indexed keys get deleted by Onyx.clear and collection sweeps. cache.set(key, value); + if (value === null || value === undefined) { + cache.addRemovedInOtherInstanceKey(key); + } if (isCollectionMember && collectionKey) { let batch = collectionBatches.get(collectionKey); diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts index 3003b5f1f..96df4627e 100644 --- a/lib/OnyxCache.ts +++ b/lib/OnyxCache.ts @@ -48,6 +48,9 @@ class OnyxCache { /** A list of keys where a nullish value has been fetched from storage before, but the key still exists in cache */ private nullishStorageKeys: Set; + /** Keys another instance removed, which explains why the cache has no value for them. They still count as uncached, unlike nullish keys. */ + private removedInOtherInstanceKeys: Set; + /** A map of cached values */ private storageMap: Record>; @@ -72,6 +75,7 @@ class OnyxCache { constructor() { this.storageKeys = new Set(); this.nullishStorageKeys = new Set(); + this.removedInOtherInstanceKeys = new Set(); this.storageMap = {}; this.pendingPromises = new Map(); this.collectionSnapshots = new Map(); @@ -87,6 +91,8 @@ class OnyxCache { 'addNullishStorageKey', 'hasNullishStorageKey', 'clearNullishStorageKeys', + 'addRemovedInOtherInstanceKey', + 'hasRemovedInOtherInstanceKey', 'set', 'drop', 'merge', @@ -153,6 +159,22 @@ class OnyxCache { this.nullishStorageKeys = new Set(); } + /** Records that another instance removed this key */ + addRemovedInOtherInstanceKey(key: OnyxKey): void { + this.removedInOtherInstanceKeys.add(key); + } + + /** Whether another instance removed this key and it hasn't changed here since */ + hasRemovedInOtherInstanceKey(key: OnyxKey): boolean { + return this.removedInOtherInstanceKeys.has(key); + } + + /** A write here replaces what the cache knew about the key being empty, including another instance's removal */ + private forgetEmptyMarks(key: OnyxKey): void { + this.nullishStorageKeys.delete(key); + this.removedInOtherInstanceKeys.delete(key); + } + /** Check whether cache has data for the given key */ hasCacheForKey(key: OnyxKey): boolean { return this.storageMap[key] !== undefined || this.hasNullishStorageKey(key); @@ -172,7 +194,7 @@ class OnyxCache { // When a key is explicitly set in cache, we can remove it from the list of nullish keys, // since it will either be set to a non nullish value or removed from the cache completely. - this.nullishStorageKeys.delete(key); + this.forgetEmptyMarks(key); const collectionKey = OnyxKeys.getCollectionKey(key); const oldValue = this.storageMap[key]; @@ -210,6 +232,7 @@ class OnyxCache { } this.storageKeys.delete(key); + this.removedInOtherInstanceKeys.delete(key); OnyxKeys.deregisterMemberKey(key); } @@ -248,7 +271,7 @@ class OnyxCache { affectedCollections.add(collectionKey); } } else { - this.nullishStorageKeys.delete(key); + this.forgetEmptyMarks(key); const existing = this.storageMap[key]; @@ -319,7 +342,7 @@ class OnyxCache { affectedCollections.add(collectionKey); } } else { - this.nullishStorageKeys.delete(key); + this.forgetEmptyMarks(key); // Per-key merge instead of spreading the entire storageMap const existing = this.storageMap[key]; diff --git a/lib/OnyxSubscriptionManager.ts b/lib/OnyxSubscriptionManager.ts index aedab2fbd..81bddac3f 100644 --- a/lib/OnyxSubscriptionManager.ts +++ b/lib/OnyxSubscriptionManager.ts @@ -19,7 +19,7 @@ type GenericListener = (value: unknown, key: OnyxKey) => void; const reportedCacheMisses = new Set(); function reportCacheMissIfStorageHasValue(key: OnyxKey): void { - if (!cache.getAllKeys().has(key) || OnyxKeys.isRamOnlyKey(key) || cache.hasNullishStorageKey(key)) { + if (!cache.getAllKeys().has(key) || OnyxKeys.isRamOnlyKey(key) || cache.hasNullishStorageKey(key) || cache.hasRemovedInOtherInstanceKey(key)) { return; } diff --git a/tests/unit/OnyxSubscriptionManagerTest.ts b/tests/unit/OnyxSubscriptionManagerTest.ts index aa8488237..bbecc4ad5 100644 --- a/tests/unit/OnyxSubscriptionManagerTest.ts +++ b/tests/unit/OnyxSubscriptionManagerTest.ts @@ -28,6 +28,7 @@ const MEMBER_2 = `${COLLECTION}2`; Onyx.init({ keys: ONYXKEYS, + shouldSyncMultipleInstances: true, }); beforeEach(() => Onyx.clear()); @@ -354,6 +355,13 @@ describe('OnyxSubscriptionManager', () => { cache.setAllKeys([key]); }; + // The handler Onyx.init registered for changes another tab wrote to storage. + const getSyncCallback = (): ((pairs: Array<[OnyxKey, unknown]>) => void) => { + const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0] as ((pairs: Array<[OnyxKey, unknown]>) => void) | undefined; + expect(syncCallback).toBeDefined(); + return syncCallback as (pairs: Array<[OnyxKey, unknown]>) => void; + }; + it('should report once when the cache has no value but storage does', async () => { await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'from-storage'); forgetValueButKeepKey(ONYXKEYS.TEST_KEY); @@ -426,6 +434,90 @@ describe('OnyxSubscriptionManager', () => { expect(logHmmmSpy).toHaveBeenCalledTimes(1); }); + it('should not read storage or report for a key another tab removed', async () => { + const syncCallback = getSyncCallback(); + const getItemMock = jest.mocked(StorageMock.getItem); + + await Onyx.set(MEMBER_1, {id: 1}); + await StorageMock.removeItem(MEMBER_1); + getItemMock.mockClear(); + syncCallback([[MEMBER_1, undefined]]); + + expect(onyxSubscriptionManager.getState(MEMBER_1)).toBeUndefined(); + await waitForPromisesToResolve(); + + expect(getItemMock).not.toHaveBeenCalled(); + expect(logHmmmSpy).not.toHaveBeenCalled(); + }); + + it('should still report a real miss on a collection after another tab removed one of its members', async () => { + const syncCallback = getSyncCallback(); + + await Onyx.set(MEMBER_1, {id: 1}); + await StorageMock.removeItem(MEMBER_1); + syncCallback([[MEMBER_1, undefined]]); + onyxSubscriptionManager.getState(MEMBER_1); + await waitForPromisesToResolve(); + + await StorageMock.setItem(MEMBER_2, {id: 2}); + forgetValueButKeepKey(MEMBER_2); + onyxSubscriptionManager.getState(MEMBER_2); + await waitForPromisesToResolve(); + + expect(logHmmmSpy).toHaveBeenCalledTimes(1); + expect(logHmmmSpy).toHaveBeenCalledWith(expect.stringContaining(COLLECTION)); + }); + + it('should not report when another tab removes a key and writes it back before this tab reads it', async () => { + // Given another tab removed a key and this tab applied that removal + const syncCallback = getSyncCallback(); + await Onyx.set(ONYXKEYS.TEST_KEY, 'first'); + await StorageMock.removeItem(ONYXKEYS.TEST_KEY); + syncCallback([[ONYXKEYS.TEST_KEY, undefined]]); + + // When that tab writes the key back, and this tab reads it before the sync for that write arrives + await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'second'); + onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY); + await waitForPromisesToResolve(); + + // Then nothing is reported, because nothing was lost + expect(logHmmmSpy).not.toHaveBeenCalled(); + }); + + it('should report a key lost after another tab removed it and it got a value again', async () => { + // Given a key another tab removed, which then got a value again + const syncCallback = getSyncCallback(); + await Onyx.set(ONYXKEYS.TEST_KEY, 'first'); + await StorageMock.removeItem(ONYXKEYS.TEST_KEY); + syncCallback([[ONYXKEYS.TEST_KEY, undefined]]); + await Onyx.set(ONYXKEYS.TEST_KEY, 'second'); + + // When the cache then has no value for it while storage still does + jest.spyOn(cache, 'get').mockReturnValueOnce(undefined); + expect(onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY)).toBeUndefined(); + await waitForPromisesToResolve(); + + // Then this is a real miss, and the old removal doesn't hide it + expect(logHmmmSpy).toHaveBeenCalledTimes(1); + }); + + it('should not report a key whose removal and re-add by another tab straddle a clear that keeps it', async () => { + // Given a key another tab removed + const syncCallback = getSyncCallback(); + await Onyx.set(ONYXKEYS.TEST_KEY, 'first'); + await StorageMock.removeItem(ONYXKEYS.TEST_KEY); + syncCallback([[ONYXKEYS.TEST_KEY, undefined]]); + + // When that tab writes it back, and this tab clears while keeping the key before that sync arrives + await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'second'); + await Onyx.clear([ONYXKEYS.TEST_KEY]); + onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY); + await waitForPromisesToResolve(); + + // Then the pending write-back isn't reported as a lost value + expect(logHmmmSpy).not.toHaveBeenCalled(); + }); + it('should not report a key the index does not know, which means init has not loaded it', async () => { await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'from-storage'); cache.drop(ONYXKEYS.TEST_KEY); diff --git a/tests/unit/onyxCacheTest.tsx b/tests/unit/onyxCacheTest.tsx index 5d6e78344..47ccd4aaf 100644 --- a/tests/unit/onyxCacheTest.tsx +++ b/tests/unit/onyxCacheTest.tsx @@ -217,6 +217,33 @@ describe('Onyx', () => { }); }); + describe('keys removed in another instance', () => { + it('should not count as cached, unlike a nullish key', () => { + // Given a key another instance removed + cache.addRemovedInOtherInstanceKey('mockKey'); + + // Then it is marked, and reads still treat the key as having nothing cached + expect(cache.hasRemovedInOtherInstanceKey('mockKey')).toBe(true); + expect(cache.hasCacheForKey('mockKey')).toBe(false); + }); + + it.each<[string, (onyxCache: typeof OnyxCache) => void]>([ + ['set', (onyxCache) => onyxCache.set('mockKey', 'value')], + ['merge', (onyxCache) => onyxCache.merge({mockKey: {a: 1}})], + ['hydrate', (onyxCache) => onyxCache.hydrate({mockKey: 'value'})], + ['drop', (onyxCache) => onyxCache.drop('mockKey')], + ])('should forget the mark on %s, because the removal no longer explains a missing value', (_name, change) => { + // Given a key another instance removed + cache.addRemovedInOtherInstanceKey('mockKey'); + + // When the key changes state here + change(cache); + + // Then the old removal is forgotten + expect(cache.hasRemovedInOtherInstanceKey('mockKey')).toBe(false); + }); + }); + describe('merge', () => { it('should create the value in cache when it does not exist', () => { // Given empty cache diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 7e465feaf..8ccefb6e7 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -4616,6 +4616,80 @@ describe('RAM-only keys should not read from storage', () => { }); }); +// A sync read can start before this instance's write reaches storage and answer after it. The answer reports the key as removed, while storage ends up holding the write. +describe('a stale removal synced from another instance', () => { + const COLLECTION = ONYX_KEYS.COLLECTION.TEST_KEY; + const MEMBER_1 = `${COLLECTION}1`; + const MEMBER_2 = `${COLLECTION}2`; + + const writeThenReceiveStaleRemoval = async (key: OnyxKey, value: unknown) => { + await Onyx.set(key, value); + const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0]; + syncCallback([[key, undefined]]); + }; + + beforeEach(async () => { + Object.assign(OnyxUtils.getDeferredInitTask(), createDeferredTask()); + Onyx.init({keys: ONYX_KEYS, shouldSyncMultipleInstances: true}); + await act(async () => waitForPromisesToResolve()); + + // Another key keeps the index non-empty, as in any running app, because getAllKeys lists storage when the index is empty and that would hide a missed key. + await Onyx.set(ONYX_KEYS.OTHER_TEST, 'other'); + }); + + afterEach(async () => { + await Onyx.clear(); + await StorageMock.clear(); + }); + + it('should still delete the key on Onyx.clear', async () => { + // Given a stored value whose key this instance was told is removed + await writeThenReceiveStaleRemoval(ONYX_KEYS.TEST_KEY, 'mine'); + + // When this instance clears, as it does on sign-out + await Onyx.clear(); + + // Then the value doesn't survive into the next session + expect(await StorageMock.getItem(ONYX_KEYS.TEST_KEY)).toBeNull(); + }); + + it('should merge into the stored value on mergeCollection instead of overwriting it', async () => { + // Given a stored member whose key this instance was told is removed + await writeThenReceiveStaleRemoval(MEMBER_1, {a: 1, b: 2}); + + // When a collection merge touches that member + await Onyx.mergeCollection(COLLECTION, {[MEMBER_1]: {c: 3}} as GenericCollection); + + // Then the stored fields are kept + expect(await StorageMock.getItem(MEMBER_1)).toEqual({a: 1, b: 2, c: 3}); + }); + + it.each<[string, () => Promise]>([ + ['multiSet', () => Onyx.multiSet({[MEMBER_1]: null})], + ['mergeCollection', () => Onyx.mergeCollection(COLLECTION, {[MEMBER_1]: null} as GenericCollection)], + ])('should delete the stored value when %s writes null', async (_name, writeNull) => { + // Given a stored member whose key this instance was told is removed + await writeThenReceiveStaleRemoval(MEMBER_1, {a: 1}); + + // When a later write removes it + await writeNull(); + + // Then the row doesn't come back on the next load + expect(await StorageMock.getItem(MEMBER_1)).toBeNull(); + }); + + it('should delete a member that setCollection leaves out', async () => { + // Given a stored member whose key this instance was told is removed + await writeThenReceiveStaleRemoval(MEMBER_1, {a: 1}); + + // When setCollection replaces the collection without it + await Onyx.setCollection(COLLECTION, {[MEMBER_2]: {b: 2}} as GenericCollection); + + // Then the row doesn't come back on the next load + expect(await StorageMock.getItem(MEMBER_1)).toBeNull(); + }); +}); + describe('get() should prefer cache over stale storage', () => { let cache: typeof OnyxCache; diff --git a/tests/unit/useOnyxTest.ts b/tests/unit/useOnyxTest.ts index 83685255c..9df4debba 100644 --- a/tests/unit/useOnyxTest.ts +++ b/tests/unit/useOnyxTest.ts @@ -5,6 +5,7 @@ import type {UseOnyxSelector} from '../../lib/useOnyx'; import type GenericCollection from '../utils/GenericCollection'; import Onyx, {useOnyx} from '../../lib'; +import cache from '../../lib/OnyxCache'; import {resetDeferredInitTask} from '../../lib/OnyxUtils'; import StorageMock from '../../lib/storage'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; @@ -25,6 +26,7 @@ Onyx.init({ keys: ONYXKEYS, skippableCollectionMemberIDs: ['skippable-id'], ramOnlyKeys: [ONYXKEYS.RAM_ONLY_KEY, ONYXKEYS.COLLECTION.RAM_ONLY_COLLECTION, ONYXKEYS.RAM_ONLY_WITH_INITIAL_VALUE], + shouldSyncMultipleInstances: true, }); beforeEach(async () => { @@ -219,6 +221,49 @@ describe('useOnyx', () => { expect(result.current[1].status).toEqual('loaded'); }); + it('should keep reporting loading for a key another tab removed while a merge to it is pending', async () => { + const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0]; + expect(syncCallback).toBeDefined(); + + await Onyx.set(ONYXKEYS.TEST_KEY, 'before'); + await StorageMock.removeItem(ONYXKEYS.TEST_KEY); + syncCallback([[ONYXKEYS.TEST_KEY, undefined]]); + Onyx.merge(ONYXKEYS.TEST_KEY, 'after'); + + const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY)); + + // Same as after a local removal: the pending merge keeps the hook loading until it lands. + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loading'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('after'); + expect(result.current[1].status).toEqual('loaded'); + }); + + it('should keep reporting loading for a known-empty key another tab removed while a merge to it is pending', async () => { + const syncCallback = (StorageMock.keepInstancesSync as jest.Mock).mock.calls.at(-1)?.[0]; + expect(syncCallback).toBeDefined(); + + // Given a key this tab knows is empty, which another tab then removes + cache.addNullishStorageKey(ONYXKEYS.TEST_KEY); + syncCallback([[ONYXKEYS.TEST_KEY, undefined]]); + + // When a merge to it is pending as a subscriber mounts + Onyx.merge(ONYXKEYS.TEST_KEY, 'after'); + const {result} = renderHook(() => useOnyx(ONYXKEYS.TEST_KEY)); + + // Then the subscriber waits for the merge instead of seeing a loaded key with no value + expect(result.current[0]).toBeUndefined(); + expect(result.current[1].status).toEqual('loading'); + + await act(async () => waitForPromisesToResolve()); + + expect(result.current[0]).toEqual('after'); + expect(result.current[1].status).toEqual('loaded'); + }); + it('should initially return undefined and then return cached value after multiple merge operations', async () => { Onyx.merge(ONYXKEYS.TEST_KEY, 'test1'); Onyx.merge(ONYXKEYS.TEST_KEY, 'test2');