diff --git a/API-INTERNAL.md b/API-INTERNAL.md index bc8c0b467..a56f5c6ad 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -111,10 +111,11 @@ provider) once so it's visible, then bounded retry without eviction.
broadcastUpdate()

Notifies subscribers and writes current value to cache

-
prepareKeyValuePairsForStorage() ⇒
+
prepareKeyValuePairsForStorage()

Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} -to an array of key-value pairs in the above format and removes key-value pairs that are being set to null

+to an array of key-value pairs in the above format, and collects the keys of null values into +keysToRemove for the caller to delete as one batch (cache drop + notification + batched storage removal).

mergeChanges(changes, existingValue)

Merges an array of changes with an existing value or creates a single change.

@@ -376,13 +377,13 @@ Notifies subscribers and writes current value to cache **Kind**: global function -## prepareKeyValuePairsForStorage() ⇒ +## prepareKeyValuePairsForStorage() Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} -to an array of key-value pairs in the above format and removes key-value pairs that are being set to null +to an array of key-value pairs in the above format, and collects the keys of null values into +`keysToRemove` for the caller to delete as one batch (cache drop + notification + batched storage removal). **Kind**: global function -**Returns**: an array of key - value pairs <[key, value]> ## mergeChanges(changes, existingValue) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 31c50c38e..9cb8ebb5c 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -64,6 +64,12 @@ function resetDiskPressureLogThrottle(): void { type OnyxMethod = ValueOf; +/** Result of `prepareKeyValuePairsForStorage`: pairs to write and keys whose `null` value marks them for removal. */ +type PreparedKeyValuePairs = { + pairs: StorageKeyValuePair[]; + keysToRemove: OnyxKey[]; +}; + // Key/value store of Onyx key and arrays of values to merge let mergeQueue: Record>> = {}; let mergeQueuePromise: Record> = {}; @@ -919,21 +925,20 @@ function hasPendingMergeForKey(key: OnyxKey): boolean { /** * Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] * This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} - * to an array of key-value pairs in the above format and removes key-value pairs that are being set to null - * - * @return an array of key - value pairs <[key, value]> + * to an array of key-value pairs in the above format, and collects the keys of null values into + * `keysToRemove` for the caller to delete as one batch (cache drop + notification + batched storage removal). */ function prepareKeyValuePairsForStorage( data: Record>, shouldRemoveNestedNulls?: boolean, replaceNullPatches?: MultiMergeReplaceNullPatches, - isProcessingCollectionUpdate?: boolean, -): StorageKeyValuePair[] { +): PreparedKeyValuePairs { const pairs: StorageKeyValuePair[] = []; + const keysToRemove: OnyxKey[] = []; for (const [key, value] of Object.entries(data)) { if (value === null) { - remove(key, isProcessingCollectionUpdate); + keysToRemove.push(key); continue; } @@ -944,7 +949,7 @@ function prepareKeyValuePairsForStorage( } } - return pairs; + return {pairs, keysToRemove}; } /** @@ -1408,7 +1413,7 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom }, {}); } - const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); + const {pairs: keyValuePairsToSet, keysToRemove} = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); // Group collection members by their parent collection key so each collection can be notified // via a single batched keysChanged() call instead of one keyChanged() per member. For each @@ -1456,6 +1461,27 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom } } + // Null keys join the same per-collection batches (as undefined) and are deleted from storage + // in one batched call below, so cross-tab sync raises a single event instead of one per key. + for (const key of keysToRemove) { + const previousValue = cache.get(key); + cache.drop(key); + + const collectionKey = OnyxKeys.getCollectionKey(key); + if (collectionKey && OnyxKeys.isCollectionMemberKey(collectionKey, key)) { + let batch = collectionBatches.get(collectionKey); + if (!batch) { + batch = {partial: {}, previous: {}}; + collectionBatches.set(collectionKey, batch); + } + batch.partial[key] = undefined; + batch.previous[key] = previousValue; + } else if (!retryAttempt) { + // Skip subscriber notification on retry — already notified on attempt 0. + keyChanged(key, undefined); + } + } + // One keysChanged() per collection — fires each collection-level subscriber once and lets // keysChanged() internally decide which individual member subscribers need notification. // Skip on retry — already notified on attempt 0 (see same-reason comment above). @@ -1470,10 +1496,16 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom // Filter out the RAM-only key value pairs, as they should not be saved to storage return !OnyxKeys.isRamOnlyKey(key); }); + const keysToRemoveFromStorage = keysToRemove.filter((key) => !OnyxKeys.isRamOnlyKey(key)); const inFlightKeys = new Set(keyValuePairsToSet.map(([key]) => key)); - return Storage.multiSet(keyValuePairsToStore) + const storagePromises = [Storage.multiSet(keyValuePairsToStore)]; + if (keysToRemoveFromStorage.length > 0) { + storagePromises.push(Storage.removeItems(keysToRemoveFromStorage)); + } + + return Promise.all(storagePromises) .then(() => StorageCircuitBreaker.recordWriteSuccess()) .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt, inFlightKeys)) .then(() => { @@ -1534,10 +1566,14 @@ function setCollectionWithRetry({collectionKey, mutableCollection[key] = null; } - const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true); + const {pairs: keyValuePairs, keysToRemove: removalCandidates} = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true); + // Removals of keys that are neither cached nor persisted are no-ops and skipped. + const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.has(key)); + // Snapshot before cache mutations so keysChanged() can diff removed members. const previousCollection = OnyxUtils.getCachedCollection(collectionKey); for (const [key, value] of keyValuePairs) cache.set(key, value); + for (const key of keysToRemove) cache.drop(key); // Skip subscriber notification on retry — already notified on attempt 0. // Collection-root subscribers re-fire on every keysChanged by contract. @@ -1553,7 +1589,13 @@ function setCollectionWithRetry({collectionKey, const inFlightKeys = new Set(keyValuePairs.map(([key]) => key)); - return Storage.multiSet(keyValuePairs) + // One batched removal = one cross-tab sync event instead of one per key. + const storagePromises = [Storage.multiSet(keyValuePairs)]; + if (keysToRemove.length > 0) { + storagePromises.push(Storage.removeItems(keysToRemove)); + } + + return Promise.all(storagePromises) .then(() => StorageCircuitBreaker.recordWriteSuccess()) .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt, inFlightKeys)) .then(() => { @@ -1612,10 +1654,14 @@ function mergeCollectionWithPatches( return getAllKeys() .then((persistedKeys) => { - // Split to keys that exist in storage and keys that don't + // Split to keys that exist in storage and keys that don't. Null members are collected + // for one batched removal below; nulls that are neither cached nor persisted are no-ops and skipped. + const keysToRemove: OnyxKey[] = []; const keys = resultCollectionKeys.filter((key) => { if (resultCollection[key] === null) { - remove(key, isProcessingCollectionUpdate); + if (cache.get(key) !== undefined || persistedKeys.has(key)) { + keysToRemove.push(key); + } return false; } return true; @@ -1658,11 +1704,11 @@ function mergeCollectionWithPatches( // When (multi-)merging the values with the existing values in storage, // we don't want to remove nested null values from the data that we pass to the storage layer, // because the storage layer uses them to remove nested keys from storage natively. - const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches); + const {pairs: keyValuePairsForExistingCollection} = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches); // We can safely remove nested null values when using (multi-)set, // because we will simply overwrite the existing values in storage. - const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection, true); + const {pairs: keyValuePairsForNewCollection} = prepareKeyValuePairsForStorage(newCollection, true); // finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates const finalMergedCollection = { @@ -1688,15 +1734,33 @@ function mergeCollectionWithPatches( // ensuring subscribers still reflect the merged data even if the subsequent storage // write fails. const previousCollection = getCachedCollection(collectionKey, existingKeys); + + // Removed members join the same keysChanged() batch (as undefined); previous values + // are snapshotted before the cache drop so keysChanged() can diff them. + const removedPreviousValues: OnyxInputKeyValueMapping = {}; + for (const key of keysToRemove) { + removedPreviousValues[key] = cache.get(key); + cache.drop(key); + } + cache.merge(finalMergedCollection); // Skip subscriber notification on retry — already notified on attempt 0. // Collection-root subscribers re-fire on every keysChanged by contract. if (!retryAttempt) { - keysChanged(collectionKey, finalMergedCollection, previousCollection); + const partialForNotify = keysToRemove.length > 0 ? {...finalMergedCollection, ...Object.fromEntries(keysToRemove.map((key) => [key, undefined]))} : finalMergedCollection; + const previousForNotify = keysToRemove.length > 0 ? {...previousCollection, ...removedPreviousValues} : previousCollection; + if (Object.keys(partialForNotify).length > 0) { + keysChanged(collectionKey, partialForNotify, previousForNotify); + } } const promises = []; + // One batched removal = one cross-tab sync event instead of one per key. + if (!OnyxKeys.isRamOnlyKey(collectionKey) && keysToRemove.length > 0) { + promises.push(Storage.removeItems(keysToRemove)); + } + // New keys go through multiSet and existing keys through multiMerge. multiMerge on a // missing key stores the value just like multiSet across all backends; splitting them lets // multiSet strip nested nulls (the merge layer keeps them to delete nested storage keys). @@ -1777,10 +1841,14 @@ function partialSetCollection({collectionKey, co return getAllKeys().then((persistedKeys) => { const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection}; const existingKeys = resultCollectionKeys.filter((key) => persistedKeys.has(key)); + const {pairs: keyValuePairs, keysToRemove: removalCandidates} = prepareKeyValuePairsForStorage(mutableCollection, true); + // Removals of keys that are neither cached nor persisted are no-ops and skipped. + const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.has(key)); + // Snapshot before cache mutations so keysChanged() can diff removed members. const previousCollection = getCachedCollection(collectionKey, existingKeys); - const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true); for (const [key, value] of keyValuePairs) cache.set(key, value); + for (const key of keysToRemove) cache.drop(key); // Skip subscriber notification on retry — already notified on attempt 0. // Collection-root subscribers re-fire on every keysChanged by contract. @@ -1795,7 +1863,13 @@ function partialSetCollection({collectionKey, co const inFlightKeys = new Set(keyValuePairs.map(([key]) => key)); - return Storage.multiSet(keyValuePairs) + // One batched removal = one cross-tab sync event instead of one per key. + const storagePromises = [Storage.multiSet(keyValuePairs)]; + if (keysToRemove.length > 0) { + storagePromises.push(Storage.removeItems(keysToRemove)); + } + + return Promise.all(storagePromises) .then(() => StorageCircuitBreaker.recordWriteSuccess()) .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt, inFlightKeys)) .then(() => { diff --git a/lib/storage/InstanceSync/index.web.ts b/lib/storage/InstanceSync/index.web.ts index b79491cab..640adcf38 100644 --- a/lib/storage/InstanceSync/index.web.ts +++ b/lib/storage/InstanceSync/index.web.ts @@ -100,6 +100,10 @@ const InstanceSync = { init: (onStorageKeysChanged: OnStorageKeysChanged, store: StorageProvider) => { storage = store; + // Coalesce storage events into one dispatch per tick: a per-key sender would otherwise re-run + // the whole notification pipeline once per key and can flood the receiving tab into unresponsiveness. + let pendingSyncKeys: Set | null = null; + // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { // Ignore events that don't originate from the SYNC_ONYX logic @@ -109,7 +113,19 @@ const InstanceSync = { const onyxKeys = parseSyncOnyxStorageEventValue(event.newValue); - storage.multiGet(onyxKeys).then((pairs) => onStorageKeysChanged(pairs)); + if (pendingSyncKeys) { + for (const onyxKey of onyxKeys) { + pendingSyncKeys.add(onyxKey); + } + return; + } + + pendingSyncKeys = new Set(onyxKeys); + setTimeout(() => { + const keys = Array.from(pendingSyncKeys ?? []); + pendingSyncKeys = null; + storage.multiGet(keys).then((pairs) => onStorageKeysChanged(pairs)); + }, 0); }); }, setItem: raiseStorageSyncEvent, diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 938a64038..ef914c9a8 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -2981,6 +2981,137 @@ describe('Onyx', () => { }); }); + describe('batched collection member removals', () => { + const routeA = `${ONYX_KEYS.COLLECTION.ROUTES}A`; + const routeB = `${ONYX_KEYS.COLLECTION.ROUTES}B`; + + it('mergeCollection deletes null members from cache and storage via one batched removeItems call', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + [routeB]: {name: 'Route B'}, + } as GenericCollection); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: null, + [routeB]: {name: 'Route B v2'}, + } as GenericCollection); + + // Per-key removals would raise one cross-tab sync event per member; the batch must persist in one call. + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).toHaveBeenCalledTimes(1); + expect(StorageMock.removeItems).toHaveBeenCalledWith([routeA]); + + expect(cache.get(routeA)).toBeUndefined(); + const keys = await OnyxUtils.getAllKeys(); + expect(keys.has(routeA)).toBe(false); + expect(keys.has(routeB)).toBe(true); + }); + + it('mergeCollection notifies member subscribers about batched removals', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + } as GenericCollection); + + let received: unknown = 'sentinel'; + connection = Onyx.connect({ + key: routeA, + callback: (value) => (received = value), + }); + await waitForPromisesToResolve(); + expect(received).toEqual({name: 'Route A'}); + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: null, + } as GenericCollection); + + expect(received).toBeUndefined(); + }); + + it('mergeCollection skips removals of members that are neither cached nor persisted', async () => { + const routeMissing = `${ONYX_KEYS.COLLECTION.ROUTES}Missing`; + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + } as GenericCollection); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + // Nulling a member that was never stored must not raise any storage removal. + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeMissing]: null, + [routeA]: {name: 'Route A v2'}, + } as GenericCollection); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).not.toHaveBeenCalled(); + }); + + it('setCollection deletes missing members via one batched removeItems call', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + [routeB]: {name: 'Route B'}, + } as GenericCollection); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + await Onyx.setCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'New Route A'}, + } as GenericCollection); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).toHaveBeenCalledTimes(1); + expect(StorageMock.removeItems).toHaveBeenCalledWith([routeB]); + + const keys = await OnyxUtils.getAllKeys(); + expect(keys.has(routeB)).toBe(false); + }); + + it('notifies member subscribers when a cached-only (RAM-only) member is removed via a batched set', async () => { + const ramKey = `${ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION}removal`; + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION, { + [ramKey]: {name: 'RAM member'}, + } as GenericCollection); + + let received: unknown = 'sentinel'; + connection = Onyx.connect({ + key: ramKey, + callback: (value) => (received = value), + }); + await waitForPromisesToResolve(); + expect(received).toEqual({name: 'RAM member'}); + + // Two set updates on members of the same collection are batched into partialSetCollection, + // where the removed member exists only in cache (RAM-only keys are never persisted). + const ramKeyOther = `${ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION}other`; + await Onyx.update([ + {onyxMethod: Onyx.METHOD.SET, key: ramKey, value: null}, + {onyxMethod: Onyx.METHOD.SET, key: ramKeyOther, value: {name: 'other'}}, + ]); + + expect(received).toBeUndefined(); + }); + + it('multiSet deletes null keys via one batched removeItems call', async () => { + await Onyx.multiSet({[ONYX_KEYS.OTHER_TEST]: 42}); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + await Onyx.multiSet({[ONYX_KEYS.OTHER_TEST]: null}); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).toHaveBeenCalledTimes(1); + expect(StorageMock.removeItems).toHaveBeenCalledWith([ONYX_KEYS.OTHER_TEST]); + expect(cache.get(ONYX_KEYS.OTHER_TEST)).toBeUndefined(); + }); + }); + describe('clear', () => { it('should handle RAM-only keys with defaults correctly during clear', async () => { // Set a value for RAM-only key diff --git a/tests/unit/storage/instanceSyncWebTest.ts b/tests/unit/storage/instanceSyncWebTest.ts index c27d5a85f..0dc750494 100644 --- a/tests/unit/storage/instanceSyncWebTest.ts +++ b/tests/unit/storage/instanceSyncWebTest.ts @@ -109,6 +109,34 @@ describe('InstanceSync (web)', () => { expect(multiGet).toHaveBeenCalledWith(['123']); }); + it('coalesces a burst of storage events into one multiGet and one dispatch', async () => { + // A tab running an older bundle emits one event per key; the burst must collapse into one batch. + storageEventHandler({key: SYNC_ONYX, newValue: 'test_1'}); + storageEventHandler({key: SYNC_ONYX, newValue: JSON.stringify(['test_2', 'test_3'])}); + storageEventHandler({key: SYNC_ONYX, newValue: 'test_2'}); + await waitForPromisesToResolve(); + + expect(multiGet).toHaveBeenCalledTimes(1); + expect(multiGet).toHaveBeenCalledWith(['test_1', 'test_2', 'test_3']); + expect(onStorageKeysChanged).toHaveBeenCalledTimes(1); + expect(onStorageKeysChanged).toHaveBeenCalledWith([ + ['test_1', 'value_of_test_1'], + ['test_2', 'value_of_test_2'], + ['test_3', 'value_of_test_3'], + ]); + }); + + it('dispatches separate batches for separate bursts', async () => { + storageEventHandler({key: SYNC_ONYX, newValue: 'test_1'}); + await waitForPromisesToResolve(); + storageEventHandler({key: SYNC_ONYX, newValue: 'test_2'}); + await waitForPromisesToResolve(); + + expect(multiGet).toHaveBeenCalledTimes(2); + expect(multiGet).toHaveBeenNthCalledWith(1, ['test_1']); + expect(multiGet).toHaveBeenNthCalledWith(2, ['test_2']); + }); + it('ignores storage events that are not SYNC_ONYX', async () => { storageEventHandler({key: 'someOtherKey', newValue: 'test_1'}); storageEventHandler({key: SYNC_ONYX, newValue: null});