From a94c759d538390817406020bc731b1c1681db0fe Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 19 Aug 2024 09:38:39 +0200 Subject: [PATCH 1/6] fix: typo --- src/libs/actions/OnyxUpdateManager/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index deb530547021..db00a55aa25b 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -75,7 +75,7 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry Date: Mon, 19 Aug 2024 09:41:13 +0200 Subject: [PATCH 2/6] fix: chrome crashes --- .../actions/OnyxUpdateManager/utils/index.ts | 31 ++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index ffc8cbd989df..22847cb5f206 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -33,37 +33,54 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdate for (const [index, update] of updateValues.entries()) { const isFirst = index === 0; + const lastUpdateID = Number(update.lastUpdateID); + const previousUpdateID = Number(update.previousUpdateID); + + // If an update is older than the last update that was applied to the client, we can skip it + // This should already be handled before, but we'll keep this as a safeguard. + if (lastUpdateID <= lastUpdateIDFromClient) { + // eslint-disable-next-line no-continue + continue; + } + // If any update's previousUpdateID doesn't match the lastUpdateID from the previous update, the deferred updates aren't chained and there's a gap. // For the first update, we need to check that the previousUpdateID of the fetched update is the same as the lastUpdateIDAppliedToClient. // For any other updates, we need to check if the previousUpdateID of the current update is found in the deferred updates. // If an update is chained, we can add it to the applicable updates. - const isChained = isFirst ? update.previousUpdateID === lastUpdateIDFromClient : !!updates[Number(update.previousUpdateID)]; + const isChained = isFirst ? previousUpdateID === lastUpdateIDFromClient : !!updates[Number(update.previousUpdateID)]; if (isChained) { // If a gap exists already, we will not add any more updates to the applicable updates. // Instead, once there are two chained updates again, we can set "firstUpdateAfterGaps" to the first update after the current gap. if (gapExists) { // If "firstUpdateAfterGaps" hasn't been set yet and there was a gap, we need to set it to the first update after all gaps. if (!firstUpdateAfterGaps) { - firstUpdateAfterGaps = Number(update.previousUpdateID); + firstUpdateAfterGaps = previousUpdateID; } } else { // If no gap exists yet, we can add the update to the applicable updates - applicableUpdates[Number(update.lastUpdateID)] = update; + applicableUpdates[lastUpdateID] = update; } } else { + // If the previousUpdateID of the update after the gap is older than the lastUpdateIDFromClient, + // we don't want to detect a missing update, since this would cause a recursion loop in "validateAndApplyDeferredUpdates". + if (previousUpdateID <= lastUpdateIDFromClient) { + // eslint-disable-next-line no-continue + continue; + } + // When we find a (new) gap, we need to set "gapExists" to true and reset the "firstUpdateAfterGaps" variable, // so that we can continue searching for the next update after all gaps gapExists = true; firstUpdateAfterGaps = undefined; // If there is a gap, it means the previous update is the latest missing update. - latestMissingUpdateID = Number(update.previousUpdateID); + latestMissingUpdateID = previousUpdateID; } } // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list, // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates. - const firstUpdateAfterGapWithFallback = firstUpdateAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID); + const firstUpdateAfterGapWithFallback = Math.max(firstUpdateAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID), lastUpdateIDFromClient); let updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists) { @@ -99,8 +116,8 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates, lastUpdateIDFromClient); - // If we detected a gap in the deferred updates, only apply the deferred updates before the gap, - // re-fetch the missing updates and then apply the remaining deferred updates after the gap + // If newer updates got applied in the meantime, we don't need to refetch for missing updates + // and can just re-trigger the "validateAndApplyDeferredUpdates" process if (latestMissingUpdateID) { Log.info('[DeferredUpdates] Gap detected in deferred updates', false, {lastUpdateIDFromClient, latestMissingUpdateID}); From c3f85a361dbfc33e9f178446a8a7b23567c1d77a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 19 Aug 2024 09:44:54 +0200 Subject: [PATCH 3/6] test: add new test for OnyxUpdateManager edge case --- tests/unit/OnyxUpdateManagerTest.ts | 27 ++++++++++++++++++++++++++- tests/utils/createOnyxMockUpdate.ts | 6 +++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 20b29ca18a56..e0b28747e22c 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -18,6 +18,7 @@ const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; const mockUpdate3 = createOnyxMockUpdate(3); +const offsetedMockUpdate3 = createOnyxMockUpdate(3, undefined, 2); const mockUpdate4 = createOnyxMockUpdate(4); const mockUpdate5 = createOnyxMockUpdate(5); const mockUpdate6 = createOnyxMockUpdate(6); @@ -193,7 +194,7 @@ describe('OnyxUpdateManager', () => { // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // validateAndApplyDeferredUpdates should be called once for the initial deferred updates // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. // The intended assertion would look like this: // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); @@ -254,4 +255,28 @@ describe('OnyxUpdateManager', () => { expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: mockUpdate7}); }); }); + + it('should only fetch missing updates that are not stale', () => { + OnyxUpdateManager.handleOnyxUpdateGap(offsetedMockUpdate3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); + + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 4. + expect(lastUpdateIDAppliedToClient).toBe(4); + + // validateAndApplyDeferredUpdates should be called once for the initial deferred updates + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // There should be only one call to applyUpdates. The call should contain all the deferred update, + // since the locally applied updates have changed in the meantime. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: offsetedMockUpdate3, 4: mockUpdate4}); + + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/tests/utils/createOnyxMockUpdate.ts b/tests/utils/createOnyxMockUpdate.ts index f8e28e364cf7..e50918b01faf 100644 --- a/tests/utils/createOnyxMockUpdate.ts +++ b/tests/utils/createOnyxMockUpdate.ts @@ -1,10 +1,10 @@ import type {OnyxUpdate} from 'react-native-onyx'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; -const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = []): OnyxUpdatesFromServer => ({ +const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = [], previousUpdateIDOffset = 1): OnyxUpdatesFromServer => ({ type: 'https', lastUpdateID, - previousUpdateID: lastUpdateID - 1, + previousUpdateID: lastUpdateID - previousUpdateIDOffset, request: { command: 'TestCommand', successData, @@ -15,7 +15,7 @@ const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = response: { jsonCode: 200, lastUpdateID, - previousUpdateID: lastUpdateID - 1, + previousUpdateID: lastUpdateID - previousUpdateIDOffset, onyxData: successData, }, }); From 9e4f20f254167d0acfb7f70f18be22311089ba3f Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 19 Aug 2024 22:04:16 +0200 Subject: [PATCH 4/6] improve splitting logic and comments --- .../actions/OnyxUpdateManager/utils/index.ts | 103 ++++++++++-------- 1 file changed, 57 insertions(+), 46 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index 22847cb5f206..a29e6397b4e0 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -16,18 +16,30 @@ Onyx.connect({ /** * In order for the deferred updates to be applied correctly in order, * we need to check if there are any gaps between deferred updates. + * If there are gaps, we need to split the deferred updates into two parts: + * 1. The applicable updates that can be applied immediately + * 2. The updates after the gaps that need to be fetched and applied first * @param updates The deferred updates to be checked for gaps - * @param clientLastUpdateID An optional lastUpdateID passed to use instead of the lastUpdateIDAppliedToClient + * @param lastUpdateIDFromClient An optional lastUpdateID passed to use instead of the lastUpdateIDAppliedToClient * @returns */ -function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdateID?: number): DetectGapAndSplitResult { - const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; +function detectGapsAndSplit(lastUpdateIDFromClient: number): DetectGapAndSplitResult { + // We only want to apply deferred updates that are newer than the last update that was applied to the client. + // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, + // so we can safely filter out any outdated deferred updates. + const pendingDeferredUpdates = DeferredOnyxUpdates.getUpdates({minUpdateID: lastUpdateIDFromClient}); - const updateValues = Object.values(updates); + // If there are no remaining deferred updates after filtering out outdated updates, + // we don't need to iterate over the deferred updates and check for gaps. + if (Object.values(pendingDeferredUpdates).length === 0) { + return {applicableUpdates: {}, updatesAfterGaps: {}, latestMissingUpdateID: undefined}; + } + + const updateValues = Object.values(pendingDeferredUpdates); const applicableUpdates: DeferredUpdatesDictionary = {}; let gapExists = false; - let firstUpdateAfterGaps: number | undefined; + let firstUpdateIDAfterGaps: number | undefined; let latestMissingUpdateID: number | undefined; for (const [index, update] of updateValues.entries()) { @@ -36,59 +48,61 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdate const lastUpdateID = Number(update.lastUpdateID); const previousUpdateID = Number(update.previousUpdateID); - // If an update is older than the last update that was applied to the client, we can skip it - // This should already be handled before, but we'll keep this as a safeguard. - if (lastUpdateID <= lastUpdateIDFromClient) { - // eslint-disable-next-line no-continue - continue; - } + // Whether the previous update (of the current update) is outdated, because there was a newer update applied to the client. + const isPreviousUpdateAlreadyApplied = previousUpdateID <= lastUpdateIDFromClient; - // If any update's previousUpdateID doesn't match the lastUpdateID from the previous update, the deferred updates aren't chained and there's a gap. - // For the first update, we need to check that the previousUpdateID of the fetched update is the same as the lastUpdateIDAppliedToClient. + // If any update's previousUpdateID doesn't match the lastUpdateID of the previous update, + // the deferred updates aren't chained and we detected a gap. + // For the first update, we need to check that the previousUpdateID of the current update is the same as the lastUpdateIDAppliedToClient. // For any other updates, we need to check if the previousUpdateID of the current update is found in the deferred updates. // If an update is chained, we can add it to the applicable updates. - const isChained = isFirst ? previousUpdateID === lastUpdateIDFromClient : !!updates[Number(update.previousUpdateID)]; - if (isChained) { - // If a gap exists already, we will not add any more updates to the applicable updates. - // Instead, once there are two chained updates again, we can set "firstUpdateAfterGaps" to the first update after the current gap. + const isChainedToPreviousUpdate = isFirst ? isPreviousUpdateAlreadyApplied : !!pendingDeferredUpdates[previousUpdateID]; + if (isChainedToPreviousUpdate) { + // If we found a gap in the deferred updates, we will not add any more updates to the applicable updates. + // Instead, if we find two chained updates, we can set "firstUpdateIDAfterGaps" to the first update after the gap. if (gapExists) { - // If "firstUpdateAfterGaps" hasn't been set yet and there was a gap, we need to set it to the first update after all gaps. - if (!firstUpdateAfterGaps) { - firstUpdateAfterGaps = previousUpdateID; + // If there was a gap, "firstUpdateIDAfterGaps" isn't set and we find two chained updates, + // we need to set "firstUpdateIDAfterGaps" to the first update after the gap. + if (!firstUpdateIDAfterGaps) { + firstUpdateIDAfterGaps = previousUpdateID; } } else { // If no gap exists yet, we can add the update to the applicable updates applicableUpdates[lastUpdateID] = update; } } else { - // If the previousUpdateID of the update after the gap is older than the lastUpdateIDFromClient, - // we don't want to detect a missing update, since this would cause a recursion loop in "validateAndApplyDeferredUpdates". - if (previousUpdateID <= lastUpdateIDFromClient) { + // If a previous update has already been applied to the client we should not detect a gap. + // This can cause a recursion loop, because "validateAndApplyDeferredUpdates" will refetch + // missing updates up to the previous update, which will then be applied again. + if (isPreviousUpdateAlreadyApplied) { // eslint-disable-next-line no-continue continue; } - // When we find a (new) gap, we need to set "gapExists" to true and reset the "firstUpdateAfterGaps" variable, - // so that we can continue searching for the next update after all gaps + // When we find a (new) gap, we initially need to set "gapExists" to true + // and reset "firstUpdateIDAfterGaps" and continue searching the first update after all gaps. gapExists = true; - firstUpdateAfterGaps = undefined; + firstUpdateIDAfterGaps = undefined; - // If there is a gap, it means the previous update is the latest missing update. + // We need to set update the latest missing update to the previous update of the current unchained update. latestMissingUpdateID = previousUpdateID; } } - // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list, - // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates. - const firstUpdateAfterGapWithFallback = Math.max(firstUpdateAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID), lastUpdateIDFromClient); - let updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists) { - updatesAfterGaps = Object.entries(updates).reduce((acc, [lastUpdateID, update]) => { - if (Number(lastUpdateID) >= firstUpdateAfterGapWithFallback) { - acc[Number(lastUpdateID)] = update; + // If there is a gap and we didn't detect two chained updates, "firstUpdateToBeAppliedAfterGap" will always be the the last deferred update. + // We will fetch all missing updates up to the previous update and can always apply the last deferred update. + const firstUpdateToBeAppliedAfterGap = firstUpdateIDAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID); + + // Add all deferred updates after the gap(s) to "updatesAfterGaps". + // If "firstUpdateToBeAppliedAfterGap" is set to the last deferred update, the array will be empty. + Object.entries(pendingDeferredUpdates).forEach(([lastUpdateID, update]) => { + if (Number(lastUpdateID) < firstUpdateToBeAppliedAfterGap) { + return; } - return acc; + + updatesAfterGaps[Number(lastUpdateID)] = update; }, {}); } @@ -104,20 +118,16 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa Log.info('[DeferredUpdates] Processing deferred updates', false, {lastUpdateIDFromClient, previousParams}); - // We only want to apply deferred updates that are newer than the last update that was applied to the client. - // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. - const pendingDeferredUpdates = DeferredOnyxUpdates.getUpdates({minUpdateID: lastUpdateIDFromClient}); + const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(lastUpdateIDFromClient); - // If there are no remaining deferred updates after filtering out outdated ones, - // we can just unpause the queue and return - if (Object.values(pendingDeferredUpdates).length === 0) { + // If there are no applicable deferred updates and no missing deferred updates, + // we don't need to apply or re-fetch any updates. We can just unpause the queue by resolving. + if (Object.values(applicableUpdates).length === 0 && latestMissingUpdateID === undefined) { return Promise.resolve(); } - const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates, lastUpdateIDFromClient); - - // If newer updates got applied in the meantime, we don't need to refetch for missing updates - // and can just re-trigger the "validateAndApplyDeferredUpdates" process + // If newer updates got applied, we don't need to refetch for missing updates + // and will re-trigger the "validateAndApplyDeferredUpdates" process if (latestMissingUpdateID) { Log.info('[DeferredUpdates] Gap detected in deferred updates', false, {lastUpdateIDFromClient, latestMissingUpdateID}); @@ -134,7 +144,8 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number, previousPa DeferredOnyxUpdates.enqueue(updatesAfterGaps, {shouldPauseSequentialQueue: false}); - // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. + // If lastUpdateIDAppliedToClient got updated, we will just retrigger the validation + // and application of the current deferred updates. if (latestMissingUpdateID <= newLastUpdateIDFromClient) { validateAndApplyDeferredUpdates(undefined, {newLastUpdateIDFromClient, latestMissingUpdateID}) .then(() => resolve(undefined)) From 911a1dc68038dc298bf4b40da332fc6cdcf6c362 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 19 Aug 2024 22:11:17 +0200 Subject: [PATCH 5/6] rename new test --- tests/unit/OnyxUpdateManagerTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index e0b28747e22c..2cfd1eec10c7 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -256,7 +256,7 @@ describe('OnyxUpdateManager', () => { }); }); - it('should only fetch missing updates that are not stale', () => { + it('should only fetch missing updates that are not outdated (older than already locally applied update)', () => { OnyxUpdateManager.handleOnyxUpdateGap(offsetedMockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); From 4ce7ef561ad3230f272c0f559419b254235968e4 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 20 Aug 2024 09:59:28 +0200 Subject: [PATCH 6/6] fix: lint error --- src/libs/actions/OnyxUpdateManager/utils/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index a29e6397b4e0..bb5dd599480e 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -89,7 +89,7 @@ function detectGapsAndSplit(lastUpdateIDFromClient: number): DetectGapAndSplitRe } } - let updatesAfterGaps: DeferredUpdatesDictionary = {}; + const updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists) { // If there is a gap and we didn't detect two chained updates, "firstUpdateToBeAppliedAfterGap" will always be the the last deferred update. // We will fetch all missing updates up to the previous update and can always apply the last deferred update.