From 51db8501e5c6811854e659f19e62ad24a593ffe7 Mon Sep 17 00:00:00 2001 From: neil-marcellini Date: Thu, 7 Nov 2024 15:10:02 -0800 Subject: [PATCH 01/20] Fix re-authentication test that should be failing --- tests/unit/NetworkTest.ts | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index e482cc3261d4..ea638aab4cf7 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -67,34 +67,24 @@ describe('NetworkTests', () => { }, }); - // Given a test user login and account ID + // And given they are signed in return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN).then(() => { expect(isOffline).toBe(false); - // Mock fetch() so that it throws a TypeError to simulate a bad network connection - global.fetch = jest.fn().mockRejectedValue(new TypeError(CONST.ERROR.FAILED_TO_FETCH)); - - const actualXhr = HttpUtils.xhr; - + // Set up mocks for the requests const mockedXhr = jest.fn(); mockedXhr .mockImplementationOnce(() => + // Given the first request is made with an expired authToken Promise.resolve({ jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, }), ) - // Fail the call to re-authenticate - .mockImplementationOnce(actualXhr) - - // The next call should still be using the old authToken - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, - }), - ) + // And the call to re-authenticate fails to fetch + .mockImplementationOnce(() => Promise.reject(new Error('Failed to fetch'))) - // Succeed the call to set a new authToken + // And there's another request to Authenticate and it succeeds .mockImplementationOnce(() => Promise.resolve({ jsonCode: CONST.JSON_CODE.SUCCESS, @@ -102,7 +92,7 @@ describe('NetworkTests', () => { }), ) - // All remaining requests should succeed + // And all remaining requests should succeed .mockImplementation(() => Promise.resolve({ jsonCode: CONST.JSON_CODE.SUCCESS, @@ -111,8 +101,10 @@ describe('NetworkTests', () => { HttpUtils.xhr = mockedXhr; - // This should first trigger re-authentication and then a Failed to fetch + // When the user opens their public profile page PersonalDetails.openPublicProfilePage(TEST_USER_ACCOUNT_ID); + + // And the network is back online to process the requests return waitForBatchedUpdates() .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) .then(() => { @@ -123,12 +115,13 @@ describe('NetworkTests', () => { return waitForBatchedUpdates(); }) .then(() => { - // Then we will eventually have 1 call to OpenPublicProfilePage and 1 calls to Authenticate - const callsToOpenPublicProfilePage = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'OpenPublicProfilePage'); + // Then there will have been 2 calls to Authenticate, one for the failed re-authentication and one retry that succeeds const callsToAuthenticate = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'Authenticate'); + expect(callsToAuthenticate.length).toBe(2); + // And two calls to openPublicProfilePage, one with the expired token and one after re-authentication + const callsToOpenPublicProfilePage = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'OpenPublicProfilePage'); expect(callsToOpenPublicProfilePage.length).toBe(1); - expect(callsToAuthenticate.length).toBe(1); }); }); }); From f275b476ef8c4aec8c31345cdb53fc9d5371acb6 Mon Sep 17 00:00:00 2001 From: neil-marcellini Date: Fri, 8 Nov 2024 08:25:59 -0800 Subject: [PATCH 02/20] WIP retry authentication with throttle --- src/ONYXKEYS.ts | 3 + src/libs/Authentication.ts | 79 +++++++++++-------------- src/libs/Middleware/Reauthentication.ts | 33 ++++++++++- src/libs/Network/SequentialQueue.ts | 12 ++-- src/libs/Network/index.ts | 14 +++++ src/libs/RequestThrottle.ts | 65 ++++++++++---------- 6 files changed, 124 insertions(+), 82 deletions(-) diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 49dd42fa8281..4773cdab8c6b 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -36,6 +36,9 @@ const ONYXKEYS = { PERSISTED_REQUESTS: 'networkRequestQueue', PERSISTED_ONGOING_REQUESTS: 'networkOngoingRequestQueue', + /** The re-authentication request to be retried as needed */ + REAUTHENTICATION_REQUEST: 'reauthenticationRequest', + /** Stores current date */ CURRENT_DATE: 'currentDate', diff --git a/src/libs/Authentication.ts b/src/libs/Authentication.ts index 34630af81733..1ab7083b2d8e 100644 --- a/src/libs/Authentication.ts +++ b/src/libs/Authentication.ts @@ -62,55 +62,48 @@ function reauthenticate(command = ''): Promise { partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, partnerUserID: credentials?.autoGeneratedLogin, partnerUserSecret: credentials?.autoGeneratedPassword, - }) - .then((response) => { - if (response.jsonCode === CONST.JSON_CODE.UNABLE_TO_RETRY) { - // If authentication fails, then the network can be unpaused - NetworkStore.setIsAuthenticating(false); + }).then((response) => { + if (response.jsonCode === CONST.JSON_CODE.UNABLE_TO_RETRY) { + // If authentication fails, then the network can be unpaused + NetworkStore.setIsAuthenticating(false); - // When a fetch() fails due to a network issue and an error is thrown we won't log the user out. Most likely they - // have a spotty connection and will need to try to reauthenticate when they come back online. We will error so it - // can be handled by callers of reauthenticate(). - throw new Error('Unable to retry Authenticate request'); - } + // When a fetch() fails due to a network issue and an error is thrown we won't log the user out. Most likely they + // have a spotty connection and will need to try to reauthenticate when they come back online. We will error so it + // can be handled by callers of reauthenticate(). + throw new Error('Unable to retry Authenticate request'); + } - // If authentication fails and we are online then log the user out - if (response.jsonCode !== 200) { - const errorMessage = ErrorUtils.getAuthenticateErrorMessage(response); - NetworkStore.setIsAuthenticating(false); - Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', { - command, - error: errorMessage, - }); - redirectToSignIn(errorMessage); - return; - } + // If authentication fails and we are online then log the user out + if (response.jsonCode !== 200) { + const errorMessage = ErrorUtils.getAuthenticateErrorMessage(response); + NetworkStore.setIsAuthenticating(false); + Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', { + command, + error: errorMessage, + }); + redirectToSignIn(errorMessage); + return; + } - // If we reauthenticated due to an expired delegate token, restore the delegate's original account. - // This is because the credentials used to reauthenticate were for the delegate's original account, and not for the account they were connected as. - if (Delegate.isConnectedAsDelegate()) { - Log.info('Reauthenticated while connected as a delegate. Restoring original account.'); - Delegate.restoreDelegateSession(response); - return; - } + // If we reauthenticated due to an expired delegate token, restore the delegate's original account. + // This is because the credentials used to reauthenticate were for the delegate's original account, and not for the account they were connected as. + if (Delegate.isConnectedAsDelegate()) { + Log.info('Reauthenticated while connected as a delegate. Restoring original account.'); + Delegate.restoreDelegateSession(response); + return; + } - // Update authToken in Onyx and in our local variables so that API requests will use the new authToken - updateSessionAuthTokens(response.authToken, response.encryptedAuthToken); + // Update authToken in Onyx and in our local variables so that API requests will use the new authToken + updateSessionAuthTokens(response.authToken, response.encryptedAuthToken); - // Note: It is important to manually set the authToken that is in the store here since any requests that are hooked into - // reauthenticate .then() will immediate post and use the local authToken. Onyx updates subscribers lately so it is not - // enough to do the updateSessionAuthTokens() call above. - NetworkStore.setAuthToken(response.authToken ?? null); + // Note: It is important to manually set the authToken that is in the store here since any requests that are hooked into + // reauthenticate .then() will immediate post and use the local authToken. Onyx updates subscribers lately so it is not + // enough to do the updateSessionAuthTokens() call above. + NetworkStore.setAuthToken(response.authToken ?? null); - // The authentication process is finished so the network can be unpaused to continue processing requests - NetworkStore.setIsAuthenticating(false); - }) - .catch((error) => { - // In case the authenticate call throws error, we need to sign user out as most likely they are missing credentials - NetworkStore.setIsAuthenticating(false); - Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', {error}); - redirectToSignIn('passwordForm.error.fallback'); - }); + // The authentication process is finished so the network can be unpaused to continue processing requests + NetworkStore.setIsAuthenticating(false); + }); } export {reauthenticate, Authenticate}; diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index 09a01e821cb2..a67214e04420 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -1,33 +1,59 @@ +import redirectToSignIn from '@libs/actions/SignInRedirect'; import * as Authentication from '@libs/Authentication'; import Log from '@libs/Log'; import * as MainQueue from '@libs/Network/MainQueue'; import * as NetworkStore from '@libs/Network/NetworkStore'; +import type {RequestError} from '@libs/Network/SequentialQueue'; import NetworkConnection from '@libs/NetworkConnection'; import * as Request from '@libs/Request'; +import RequestThrottle from '@libs/RequestThrottle'; import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; import type Middleware from './types'; // We store a reference to the active authentication request so that we are only ever making one request to authenticate at a time. let isAuthenticating: Promise | null = null; +const reauthThrottle = new RequestThrottle(); + function reauthenticate(commandName?: string): Promise { if (isAuthenticating) { return isAuthenticating; } - isAuthenticating = Authentication.reauthenticate(commandName) + const reauthRequest = { + commandName, + }; + Onyx.set(ONYXKEYS.REAUTHENTICATION_REQUEST, reauthRequest); + + isAuthenticating = retryReauthenticate(commandName) .then((response) => { - isAuthenticating = null; return response; }) .catch((error) => { - isAuthenticating = null; throw error; + }) + .finally(() => { + Onyx.set(CONST.ONYXKEYS.REAUTHENTICATION_REQUEST, null); + isAuthenticating = null; }); return isAuthenticating; } +function retryReauthenticate(commandName?: string): Promise { + return Authentication.reauthenticate(commandName).catch((error: RequestError) => { + return reauthThrottle + .sleep(error, 'Authenticate') + .then(() => retryReauthenticate(commandName)) + .catch(() => { + NetworkStore.setIsAuthenticating(false); + Log.hmmm('Redirecting to Sign In because we failed to reauthenticate after multiple attempts', {error}); + redirectToSignIn('passwordForm.error.fallback'); + }); + }); +} + const Reauthentication: Middleware = (response, request, isFromSequentialQueue) => response .then((data) => { @@ -118,3 +144,4 @@ const Reauthentication: Middleware = (response, request, isFromSequentialQueue) }); export default Reauthentication; +export {reauthenticate}; diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 643ed64ae7f6..ec07d315a608 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -2,7 +2,7 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; import Log from '@libs/Log'; import * as Request from '@libs/Request'; -import * as RequestThrottle from '@libs/RequestThrottle'; +import RequestThrottle from '@libs/RequestThrottle'; import * as PersistedRequests from '@userActions/PersistedRequests'; import * as QueuedOnyxUpdates from '@userActions/QueuedOnyxUpdates'; import CONST from '@src/CONST'; @@ -28,6 +28,7 @@ resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; +const requestThrottle = new RequestThrottle(); /** * Puts the queue into a paused state so that no requests will be processed @@ -99,7 +100,7 @@ function process(): Promise { Log.info('[SequentialQueue] Removing persisted request because it was processed successfully.', false, {request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - RequestThrottle.clear(); + requestThrottle.clear(); return process(); }) .catch((error: RequestError) => { @@ -108,17 +109,18 @@ function process(): Promise { if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) { Log.info("[SequentialQueue] Removing persisted request because it failed and doesn't need to be retried.", false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - RequestThrottle.clear(); + requestThrottle.clear(); return process(); } PersistedRequests.rollbackOngoingRequest(); - return RequestThrottle.sleep(error, requestToProcess.command) + return requestThrottle + .sleep(error, requestToProcess.command) .then(process) .catch(() => { Onyx.update(requestToProcess.failureData ?? []); Log.info('[SequentialQueue] Removing persisted request because it failed too many times.', false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - RequestThrottle.clear(); + requestThrottle.clear(); return process(); }); }); diff --git a/src/libs/Network/index.ts b/src/libs/Network/index.ts index 2adb4a2da4c2..2a600d5d51de 100644 --- a/src/libs/Network/index.ts +++ b/src/libs/Network/index.ts @@ -1,5 +1,8 @@ +import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; +import {reauthenticate} from '@libs/Middleware/Reauthentication'; import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; @@ -12,6 +15,17 @@ ActiveClientManager.isReady().then(() => { // Start main queue and process once every n ms delay setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); + + // If a reauthentication request is set make sure it is processed + Onyx.connect({ + key: ONYXKEYS.REAUTHENTICATION_REQUEST, + callback: (request) => { + if (!request) { + return; + } + reauthenticate(request.commandName); + }, + }); }); /** diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts index 3bbc82ff5b45..8a6673c22a92 100644 --- a/src/libs/RequestThrottle.ts +++ b/src/libs/RequestThrottle.ts @@ -3,41 +3,44 @@ import Log from './Log'; import type {RequestError} from './Network/SequentialQueue'; import {generateRandomInt} from './NumberUtils'; -let requestWaitTime = 0; -let requestRetryCount = 0; +class RequestThrottle { + private requestWaitTime = 0; -function clear() { - requestWaitTime = 0; - requestRetryCount = 0; - Log.info(`[RequestThrottle] in clear()`); -} + private requestRetryCount = 0; -function getRequestWaitTime() { - if (requestWaitTime) { - requestWaitTime = Math.min(requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS); - } else { - requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); + clear() { + this.requestWaitTime = 0; + this.requestRetryCount = 0; + Log.info(`[RequestThrottle] in clear()`); } - return requestWaitTime; -} -function getLastRequestWaitTime() { - return requestWaitTime; -} - -function sleep(error: RequestError, command: string): Promise { - requestRetryCount++; - return new Promise((resolve, reject) => { - if (requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { - const currentRequestWaitTime = getRequestWaitTime(); - Log.info( - `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${requestRetryCount}. Wait time: ${currentRequestWaitTime}`, - ); - setTimeout(resolve, currentRequestWaitTime); - return; + getRequestWaitTime() { + if (this.requestWaitTime) { + this.requestWaitTime = Math.min(this.requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS); + } else { + this.requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); } - reject(); - }); + return this.requestWaitTime; + } + + getLastRequestWaitTime() { + return this.requestWaitTime; + } + + sleep(error: RequestError, command: string): Promise { + this.requestRetryCount++; + return new Promise((resolve, reject) => { + if (this.requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { + const currentRequestWaitTime = this.getRequestWaitTime(); + Log.info( + `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`, + ); + setTimeout(resolve, currentRequestWaitTime); + } else { + reject(); + } + }); + } } -export {clear, getRequestWaitTime, sleep, getLastRequestWaitTime}; +export default RequestThrottle; From 7ffdbbeed84bdd52c6e3413ea6f3e97a163caa5b Mon Sep 17 00:00:00 2001 From: neil-marcellini Date: Fri, 8 Nov 2024 08:33:39 -0800 Subject: [PATCH 03/20] Fix types --- src/ONYXKEYS.ts | 1 + src/libs/Authentication.ts | 6 +----- src/libs/Middleware/Reauthentication.ts | 5 ++++- tests/unit/APITest.ts | 8 +++++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 4773cdab8c6b..08feab508556 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -891,6 +891,7 @@ type OnyxValuesMapping = { [ONYXKEYS.IS_SIDEBAR_LOADED]: boolean; [ONYXKEYS.PERSISTED_REQUESTS]: OnyxTypes.Request[]; [ONYXKEYS.PERSISTED_ONGOING_REQUESTS]: OnyxTypes.Request; + [ONYXKEYS.REAUTHENTICATION_REQUEST]: OnyxTypes.Request; [ONYXKEYS.CURRENT_DATE]: string; [ONYXKEYS.CREDENTIALS]: OnyxTypes.Credentials; [ONYXKEYS.STASHED_CREDENTIALS]: OnyxTypes.Credentials; diff --git a/src/libs/Authentication.ts b/src/libs/Authentication.ts index 1ab7083b2d8e..5e7b00472471 100644 --- a/src/libs/Authentication.ts +++ b/src/libs/Authentication.ts @@ -64,12 +64,8 @@ function reauthenticate(command = ''): Promise { partnerUserSecret: credentials?.autoGeneratedPassword, }).then((response) => { if (response.jsonCode === CONST.JSON_CODE.UNABLE_TO_RETRY) { - // If authentication fails, then the network can be unpaused - NetworkStore.setIsAuthenticating(false); - // When a fetch() fails due to a network issue and an error is thrown we won't log the user out. Most likely they - // have a spotty connection and will need to try to reauthenticate when they come back online. We will error so it - // can be handled by callers of reauthenticate(). + // have a spotty connection and will need to retry reauthenticate when they come back online. Error so it can be handled by the retry mechanism. throw new Error('Unable to retry Authenticate request'); } diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index a67214e04420..859dfa01697a 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -1,3 +1,4 @@ +import Onyx from 'react-native-onyx'; import redirectToSignIn from '@libs/actions/SignInRedirect'; import * as Authentication from '@libs/Authentication'; import Log from '@libs/Log'; @@ -24,6 +25,7 @@ function reauthenticate(commandName?: string): Promise { const reauthRequest = { commandName, }; + // eslint-disable-next-line rulesdir/prefer-actions-set-data Onyx.set(ONYXKEYS.REAUTHENTICATION_REQUEST, reauthRequest); isAuthenticating = retryReauthenticate(commandName) @@ -34,7 +36,8 @@ function reauthenticate(commandName?: string): Promise { throw error; }) .finally(() => { - Onyx.set(CONST.ONYXKEYS.REAUTHENTICATION_REQUEST, null); + // eslint-disable-next-line rulesdir/prefer-actions-set-data + Onyx.set(ONYXKEYS.REAUTHENTICATION_REQUEST, null); isAuthenticating = null; }); diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 14c4cadcb26d..bc4b650fb6e5 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -9,7 +9,7 @@ import * as MainQueue from '@src/libs/Network/MainQueue'; import * as NetworkStore from '@src/libs/Network/NetworkStore'; import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; import * as Request from '@src/libs/Request'; -import * as RequestThrottle from '@src/libs/RequestThrottle'; +import RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx'; import * as TestHelper from '../utils/TestHelper'; @@ -39,6 +39,7 @@ type XhrCalls = Array<{ }>; const originalXHR = HttpUtils.xhr; +const requestThrottle = new RequestThrottle(); beforeEach(() => { global.fetch = TestHelper.getGlobalFetchMock(); @@ -47,6 +48,7 @@ beforeEach(() => { MainQueue.clear(); HttpUtils.cancelPendingRequests(); PersistedRequests.clear(); + requestThrottle.clear(); NetworkStore.checkRequiredData(); // Wait for any Log command to finish and Onyx to fully clear @@ -242,7 +244,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, RequestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); }); }) .then(() => { @@ -255,7 +257,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, RequestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); }).then(waitForBatchedUpdates); }) .then(() => { From f5da09521939008c1974b8943dea06dcc75e74ed Mon Sep 17 00:00:00 2001 From: neil-marcellini Date: Fri, 8 Nov 2024 10:45:28 -0800 Subject: [PATCH 04/20] Try advancing timers past throttle --- tests/unit/NetworkTest.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index ea638aab4cf7..bb2738416440 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -111,7 +111,8 @@ describe('NetworkTests', () => { expect(isOffline).toBe(false); // Advance the network request queue by 1 second so that it can realize it's back online - jest.advanceTimersByTime(CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); + // And advance past the retry delay + jest.advanceTimersByTime(CONST.NETWORK.PROCESS_REQUEST_DELAY_MS + CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); return waitForBatchedUpdates(); }) .then(() => { From 225ad109708674a3d5722be799b5f9020fccce57 Mon Sep 17 00:00:00 2001 From: neil-marcellini Date: Fri, 8 Nov 2024 13:47:43 -0800 Subject: [PATCH 05/20] Comment out shit causing a circular dependency --- src/libs/Network/index.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libs/Network/index.ts b/src/libs/Network/index.ts index 2a600d5d51de..668a7038e706 100644 --- a/src/libs/Network/index.ts +++ b/src/libs/Network/index.ts @@ -1,6 +1,6 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; -import {reauthenticate} from '@libs/Middleware/Reauthentication'; +// import {reauthenticate} from '@libs/Middleware/Reauthentication'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; @@ -17,15 +17,15 @@ ActiveClientManager.isReady().then(() => { setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); // If a reauthentication request is set make sure it is processed - Onyx.connect({ - key: ONYXKEYS.REAUTHENTICATION_REQUEST, - callback: (request) => { - if (!request) { - return; - } - reauthenticate(request.commandName); - }, - }); + // Onyx.connect({ + // key: ONYXKEYS.REAUTHENTICATION_REQUEST, + // callback: (request) => { + // if (!request) { + // return; + // } + // // reauthenticate(request.commandName); + // }, + // }); }); /** From 876ff5a6b0319292b9ccce64678dc19e24b40ce1 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Mon, 18 Nov 2024 22:41:43 +0200 Subject: [PATCH 06/20] revert reauth test --- tests/unit/NetworkTest.ts | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index bb2738416440..e482cc3261d4 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -67,24 +67,34 @@ describe('NetworkTests', () => { }, }); - // And given they are signed in + // Given a test user login and account ID return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN).then(() => { expect(isOffline).toBe(false); - // Set up mocks for the requests + // Mock fetch() so that it throws a TypeError to simulate a bad network connection + global.fetch = jest.fn().mockRejectedValue(new TypeError(CONST.ERROR.FAILED_TO_FETCH)); + + const actualXhr = HttpUtils.xhr; + const mockedXhr = jest.fn(); mockedXhr .mockImplementationOnce(() => - // Given the first request is made with an expired authToken Promise.resolve({ jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, }), ) - // And the call to re-authenticate fails to fetch - .mockImplementationOnce(() => Promise.reject(new Error('Failed to fetch'))) + // Fail the call to re-authenticate + .mockImplementationOnce(actualXhr) + + // The next call should still be using the old authToken + .mockImplementationOnce(() => + Promise.resolve({ + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, + }), + ) - // And there's another request to Authenticate and it succeeds + // Succeed the call to set a new authToken .mockImplementationOnce(() => Promise.resolve({ jsonCode: CONST.JSON_CODE.SUCCESS, @@ -92,7 +102,7 @@ describe('NetworkTests', () => { }), ) - // And all remaining requests should succeed + // All remaining requests should succeed .mockImplementation(() => Promise.resolve({ jsonCode: CONST.JSON_CODE.SUCCESS, @@ -101,28 +111,24 @@ describe('NetworkTests', () => { HttpUtils.xhr = mockedXhr; - // When the user opens their public profile page + // This should first trigger re-authentication and then a Failed to fetch PersonalDetails.openPublicProfilePage(TEST_USER_ACCOUNT_ID); - - // And the network is back online to process the requests return waitForBatchedUpdates() .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) .then(() => { expect(isOffline).toBe(false); // Advance the network request queue by 1 second so that it can realize it's back online - // And advance past the retry delay - jest.advanceTimersByTime(CONST.NETWORK.PROCESS_REQUEST_DELAY_MS + CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); + jest.advanceTimersByTime(CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); return waitForBatchedUpdates(); }) .then(() => { - // Then there will have been 2 calls to Authenticate, one for the failed re-authentication and one retry that succeeds + // Then we will eventually have 1 call to OpenPublicProfilePage and 1 calls to Authenticate + const callsToOpenPublicProfilePage = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'OpenPublicProfilePage'); const callsToAuthenticate = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'Authenticate'); - expect(callsToAuthenticate.length).toBe(2); - // And two calls to openPublicProfilePage, one with the expired token and one after re-authentication - const callsToOpenPublicProfilePage = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'OpenPublicProfilePage'); expect(callsToOpenPublicProfilePage.length).toBe(1); + expect(callsToAuthenticate.length).toBe(1); }); }); }); From 80d634b4a2f10c4063ea7f5002c7eedda210a12b Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Mon, 18 Nov 2024 22:56:07 +0200 Subject: [PATCH 07/20] revert RequestThrottle to be function based --- src/libs/Middleware/Reauthentication.ts | 7 +-- src/libs/Network/SequentialQueue.ts | 12 ++--- src/libs/RequestThrottle.ts | 65 ++++++++++++------------- tests/unit/APITest.ts | 9 ++-- 4 files changed, 42 insertions(+), 51 deletions(-) diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index 859dfa01697a..87f1d3d68034 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -7,16 +7,14 @@ import * as NetworkStore from '@libs/Network/NetworkStore'; import type {RequestError} from '@libs/Network/SequentialQueue'; import NetworkConnection from '@libs/NetworkConnection'; import * as Request from '@libs/Request'; -import RequestThrottle from '@libs/RequestThrottle'; import CONST from '@src/CONST'; +import * as RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type Middleware from './types'; // We store a reference to the active authentication request so that we are only ever making one request to authenticate at a time. let isAuthenticating: Promise | null = null; -const reauthThrottle = new RequestThrottle(); - function reauthenticate(commandName?: string): Promise { if (isAuthenticating) { return isAuthenticating; @@ -46,8 +44,7 @@ function reauthenticate(commandName?: string): Promise { function retryReauthenticate(commandName?: string): Promise { return Authentication.reauthenticate(commandName).catch((error: RequestError) => { - return reauthThrottle - .sleep(error, 'Authenticate') + return RequestThrottle.sleep(error, 'Authenticate') .then(() => retryReauthenticate(commandName)) .catch(() => { NetworkStore.setIsAuthenticating(false); diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index ec07d315a608..cdf9b075ec44 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -2,10 +2,10 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; import Log from '@libs/Log'; import * as Request from '@libs/Request'; -import RequestThrottle from '@libs/RequestThrottle'; import * as PersistedRequests from '@userActions/PersistedRequests'; import * as QueuedOnyxUpdates from '@userActions/QueuedOnyxUpdates'; import CONST from '@src/CONST'; +import * as RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type OnyxRequest from '@src/types/onyx/Request'; import type {ConflictData} from '@src/types/onyx/Request'; @@ -28,7 +28,6 @@ resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; -const requestThrottle = new RequestThrottle(); /** * Puts the queue into a paused state so that no requests will be processed @@ -100,7 +99,7 @@ function process(): Promise { Log.info('[SequentialQueue] Removing persisted request because it was processed successfully.', false, {request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - requestThrottle.clear(); + RequestThrottle.clear(); return process(); }) .catch((error: RequestError) => { @@ -109,18 +108,17 @@ function process(): Promise { if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) { Log.info("[SequentialQueue] Removing persisted request because it failed and doesn't need to be retried.", false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - requestThrottle.clear(); + RequestThrottle.clear(); return process(); } PersistedRequests.rollbackOngoingRequest(); - return requestThrottle - .sleep(error, requestToProcess.command) + return RequestThrottle.sleep(error, requestToProcess.command) .then(process) .catch(() => { Onyx.update(requestToProcess.failureData ?? []); Log.info('[SequentialQueue] Removing persisted request because it failed too many times.', false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - requestThrottle.clear(); + RequestThrottle.clear(); return process(); }); }); diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts index 8a6673c22a92..3bbc82ff5b45 100644 --- a/src/libs/RequestThrottle.ts +++ b/src/libs/RequestThrottle.ts @@ -3,44 +3,41 @@ import Log from './Log'; import type {RequestError} from './Network/SequentialQueue'; import {generateRandomInt} from './NumberUtils'; -class RequestThrottle { - private requestWaitTime = 0; +let requestWaitTime = 0; +let requestRetryCount = 0; - private requestRetryCount = 0; +function clear() { + requestWaitTime = 0; + requestRetryCount = 0; + Log.info(`[RequestThrottle] in clear()`); +} - clear() { - this.requestWaitTime = 0; - this.requestRetryCount = 0; - Log.info(`[RequestThrottle] in clear()`); - } - - getRequestWaitTime() { - if (this.requestWaitTime) { - this.requestWaitTime = Math.min(this.requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS); - } else { - this.requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); - } - return this.requestWaitTime; +function getRequestWaitTime() { + if (requestWaitTime) { + requestWaitTime = Math.min(requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS); + } else { + requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); } + return requestWaitTime; +} - getLastRequestWaitTime() { - return this.requestWaitTime; - } +function getLastRequestWaitTime() { + return requestWaitTime; +} - sleep(error: RequestError, command: string): Promise { - this.requestRetryCount++; - return new Promise((resolve, reject) => { - if (this.requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { - const currentRequestWaitTime = this.getRequestWaitTime(); - Log.info( - `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`, - ); - setTimeout(resolve, currentRequestWaitTime); - } else { - reject(); - } - }); - } +function sleep(error: RequestError, command: string): Promise { + requestRetryCount++; + return new Promise((resolve, reject) => { + if (requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { + const currentRequestWaitTime = getRequestWaitTime(); + Log.info( + `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${requestRetryCount}. Wait time: ${currentRequestWaitTime}`, + ); + setTimeout(resolve, currentRequestWaitTime); + return; + } + reject(); + }); } -export default RequestThrottle; +export {clear, getRequestWaitTime, sleep, getLastRequestWaitTime}; diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index bc4b650fb6e5..5b6947861dbe 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -9,7 +9,7 @@ import * as MainQueue from '@src/libs/Network/MainQueue'; import * as NetworkStore from '@src/libs/Network/NetworkStore'; import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; import * as Request from '@src/libs/Request'; -import RequestThrottle from '@src/libs/RequestThrottle'; +import * as RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx'; import * as TestHelper from '../utils/TestHelper'; @@ -39,7 +39,6 @@ type XhrCalls = Array<{ }>; const originalXHR = HttpUtils.xhr; -const requestThrottle = new RequestThrottle(); beforeEach(() => { global.fetch = TestHelper.getGlobalFetchMock(); @@ -48,7 +47,7 @@ beforeEach(() => { MainQueue.clear(); HttpUtils.cancelPendingRequests(); PersistedRequests.clear(); - requestThrottle.clear(); + RequestThrottle.clear(); NetworkStore.checkRequiredData(); // Wait for any Log command to finish and Onyx to fully clear @@ -244,7 +243,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, RequestThrottle.getLastRequestWaitTime()); }); }) .then(() => { @@ -257,7 +256,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, RequestThrottle.getLastRequestWaitTime()); }).then(waitForBatchedUpdates); }) .then(() => { From ff16617bf4b322d0719c592f9671e388276f4fa1 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Mon, 18 Nov 2024 23:24:58 +0200 Subject: [PATCH 08/20] fix flaky network tests --- src/libs/Middleware/Reauthentication.ts | 6 ++++- src/libs/Network/SequentialQueue.ts | 12 +++++++++- src/libs/Network/index.ts | 30 ++++++++++--------------- tests/unit/NetworkTest.ts | 26 ++++++++++++++++----- 4 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index 87f1d3d68034..29df3b7f0b50 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -54,6 +54,10 @@ function retryReauthenticate(commandName?: string): Promise { }); } +function resetReauthentication(): void { + isAuthenticating = null; +} + const Reauthentication: Middleware = (response, request, isFromSequentialQueue) => response .then((data) => { @@ -144,4 +148,4 @@ const Reauthentication: Middleware = (response, request, isFromSequentialQueue) }); export default Reauthentication; -export {reauthenticate}; +export {reauthenticate, resetReauthentication}; diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index cdf9b075ec44..68c1be0f32a5 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -271,5 +271,15 @@ function waitForIdle(): Promise { return isReadyPromise; } -export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process}; +function resetQueue(): void { + isSequentialQueueRunning = false; + currentRequestPromise = null; + isQueuePaused = false; + isReadyPromise = new Promise((resolve) => { + resolveIsReadyPromise = resolve; + }); + resolveIsReadyPromise?.(); +} + +export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process, resetQueue}; export type {RequestError}; diff --git a/src/libs/Network/index.ts b/src/libs/Network/index.ts index 668a7038e706..0ad415b74048 100644 --- a/src/libs/Network/index.ts +++ b/src/libs/Network/index.ts @@ -1,33 +1,30 @@ -import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; // import {reauthenticate} from '@libs/Middleware/Reauthentication'; import CONST from '@src/CONST'; -import ONYXKEYS from '@src/ONYXKEYS'; import type {Request} from '@src/types/onyx'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; import * as MainQueue from './MainQueue'; import * as SequentialQueue from './SequentialQueue'; +let processQueueInterval: NodeJS.Timer; + // We must wait until the ActiveClientManager is ready so that we ensure only the "leader" tab processes any persisted requests ActiveClientManager.isReady().then(() => { SequentialQueue.flush(); // Start main queue and process once every n ms delay - setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); - - // If a reauthentication request is set make sure it is processed - // Onyx.connect({ - // key: ONYXKEYS.REAUTHENTICATION_REQUEST, - // callback: (request) => { - // if (!request) { - // return; - // } - // // reauthenticate(request.commandName); - // }, - // }); + processQueueInterval = setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); }); +// Clear interval +function clearProcessQueueInterval() { + if (!processQueueInterval) { + return; + } + clearInterval(processQueueInterval as unknown as number); +} + /** * Perform a queued post request */ @@ -69,7 +66,4 @@ function post(command: string, data: Record = {}, type = CONST. }); } -export { - // eslint-disable-next-line import/prefer-default-export - post, -}; +export {post, clearProcessQueueInterval}; diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index e482cc3261d4..b403e2f274e6 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -2,6 +2,7 @@ import type {Mock} from 'jest-mock'; import type {OnyxEntry} from 'react-native-onyx'; import MockedOnyx from 'react-native-onyx'; import * as App from '@libs/actions/App'; +import {resetReauthentication} from '@libs/Middleware/Reauthentication'; import CONST from '@src/CONST'; import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager'; import * as PersistedRequests from '@src/libs/actions/PersistedRequests'; @@ -12,6 +13,7 @@ import Log from '@src/libs/Log'; import * as Network from '@src/libs/Network'; import * as MainQueue from '@src/libs/Network/MainQueue'; import * as NetworkStore from '@src/libs/Network/NetworkStore'; +import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; import NetworkConnection from '@src/libs/NetworkConnection'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Session as OnyxSession} from '@src/types/onyx'; @@ -35,15 +37,27 @@ const originalXHR = HttpUtils.xhr; beforeEach(() => { global.fetch = TestHelper.getGlobalFetchMock(); HttpUtils.xhr = originalXHR; + + // Reset any pending requests MainQueue.clear(); HttpUtils.cancelPendingRequests(); NetworkStore.checkRequiredData(); - - // Wait for any Log command to finish and Onyx to fully clear - return waitForBatchedUpdates() - .then(() => PersistedRequests.clear()) - .then(() => Onyx.clear()) - .then(waitForBatchedUpdates); + NetworkStore.setIsAuthenticating(false); + resetReauthentication(); + Network.clearProcessQueueInterval(); + SequentialQueue.resetQueue(); + + return Promise.all([SequentialQueue.waitForIdle(), waitForBatchedUpdates(), PersistedRequests.clear(), Onyx.clear()]) + .then(() => { + return waitForBatchedUpdates(); + }) + .then( + () => + // Wait for all async operations to complete + new Promise((resolve) => { + setTimeout(resolve, 100); + }), + ); }); afterEach(() => { From c9cb455fd60ed449728431f7ed3b0ee75cab5961 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Mon, 18 Nov 2024 23:34:02 +0200 Subject: [PATCH 09/20] add improved reauth while offline test --- tests/unit/NetworkTest.ts | 93 +++++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 39 deletions(-) diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index b403e2f274e6..9607487b9e93 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -17,6 +17,7 @@ import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; import NetworkConnection from '@src/libs/NetworkConnection'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Session as OnyxSession} from '@src/types/onyx'; +import type OnyxNetwork from '@src/types/onyx/Network'; import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx'; import * as TestHelper from '../utils/TestHelper'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; @@ -148,77 +149,91 @@ describe('NetworkTests', () => { }); test('failing to reauthenticate while offline should not log out user', async () => { + // 1. Setup Phase - Initialize test user and state variables const TEST_USER_LOGIN = 'test@testguy.com'; const TEST_USER_ACCOUNT_ID = 1; + const defaultTimeout = 1000; - let session: OnyxEntry; + let sessionState: OnyxEntry; + let networkState: OnyxEntry; + + // Set up listeners for session and network state changes Onyx.connect({ key: ONYXKEYS.SESSION, - callback: (val) => (session = val), + callback: (val) => (sessionState = val), }); Onyx.connect({ key: ONYXKEYS.NETWORK, + callback: (val) => (networkState = val), }); + // Sign in test user and wait for updates await TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN); await waitForBatchedUpdates(); - expect(session?.authToken).not.toBeUndefined(); - - // Turn off the network - await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}); + const initialAuthToken = sessionState?.authToken; + expect(initialAuthToken).toBeDefined(); - const mockedXhr = jest.fn(); - mockedXhr - // Call ReconnectApp with an expired token + // 2. Mock Setup Phase - Configure XHR mocks for the test sequence + const mockedXhr = jest + .fn() + // First mock: ReconnectApp will fail with NOT_AUTHENTICATED .mockImplementationOnce(() => Promise.resolve({ jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, }), ) - // Call Authenticate - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.SUCCESS, - authToken: 'newAuthToken', - }), - ) - // Call ReconnectApp again, it should connect with a new token - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.SUCCESS, - }), - ); + // Second mock: Authenticate with network check and delay + .mockImplementationOnce(() => { + // Check network state immediately + if (networkState?.isOffline) { + return Promise.reject(new Error('Network request failed')); + } + + // create a delayed response. Timeout will expire after the second App.reconnectApp(); + return new Promise((_, reject) => { + setTimeout(() => { + reject(new Error('Network request failed')); + }, defaultTimeout); + }); + }); HttpUtils.xhr = mockedXhr; - // Initiate the requests + // 3. Test Execution Phase - Start with online network + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); + + // Trigger reconnect which will fail due to expired token App.confirmReadyToOpenApp(); App.reconnectApp(); await waitForBatchedUpdates(); - // Turn the network back online - await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); - - // Filter requests results by request name - const reconnectResults = (HttpUtils.xhr as Mock).mock.results.filter((_, index) => (HttpUtils.xhr as Mock)?.mock?.calls?.at(index)?.[0] === 'ReconnectApp'); - const authenticateResults = (HttpUtils.xhr as Mock).mock.results.filter((_, index) => (HttpUtils.xhr as Mock)?.mock?.calls?.at(index)?.[0] === 'Authenticate'); + // 4. First API Call Verification - Check ReconnectApp + const firstCall = mockedXhr.mock.calls.at(0) as [string, Record]; + expect(firstCall[0]).toBe('ReconnectApp'); - // Get the response code of Authenticate call - const authenticateResponse = await (authenticateResults?.at(0)?.value as Promise<{jsonCode: string}>); + // 5. Authentication Start - Verify authenticate was triggered + await waitForBatchedUpdates(); + const secondCall = mockedXhr.mock.calls.at(1) as [string, Record]; + expect(secondCall[0]).toBe('Authenticate'); - // Get the response code of the second Reconnect call - const reconnectResponse = await (reconnectResults?.at(1)?.value as Promise<{jsonCode: string}>); + // 6. Network State Change - Set offline and back online while authenticate is pending + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}); + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); - // Authenticate request should return 200 - expect(authenticateResponse.jsonCode).toBe(CONST.JSON_CODE.SUCCESS); + // Trigger another reconnect due to network change + App.confirmReadyToOpenApp(); + App.reconnectApp(); + await waitForBatchedUpdates(); - // The second ReconnectApp should return 200 - expect(reconnectResponse.jsonCode).toBe(CONST.JSON_CODE.SUCCESS); + // 7. Wait and Verify - Wait for authenticate timeout and verify session + await new Promise((resolve) => { + setTimeout(resolve, defaultTimeout + 100); + }); - // check if the user is still logged in - expect(session?.authToken).not.toBeUndefined(); + // Verify the session remained intact and wasn't cleared + expect(sessionState?.authToken).toBe(initialAuthToken); }); test('consecutive API calls eventually succeed when authToken is expired', () => { From db950710410ab8da968da496c0a87ad988070110 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Mon, 18 Nov 2024 23:43:45 +0200 Subject: [PATCH 10/20] cleanup code --- src/libs/Network/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/Network/index.ts b/src/libs/Network/index.ts index 0ad415b74048..d91ea8c02553 100644 --- a/src/libs/Network/index.ts +++ b/src/libs/Network/index.ts @@ -1,5 +1,4 @@ import * as ActiveClientManager from '@libs/ActiveClientManager'; -// import {reauthenticate} from '@libs/Middleware/Reauthentication'; import CONST from '@src/CONST'; import type {Request} from '@src/types/onyx'; import type Response from '@src/types/onyx/Response'; From 2a656a8caa4cdc913bfba6c87a94efa4ac399cc6 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Tue, 19 Nov 2024 13:21:23 +0200 Subject: [PATCH 11/20] fix typo --- tests/actions/ReportTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts index c279079b995b..1cd17e33829d 100644 --- a/tests/actions/ReportTest.ts +++ b/tests/actions/ReportTest.ts @@ -55,7 +55,7 @@ describe('actions/Report', () => { const promise = Onyx.clear().then(jest.useRealTimers); if (getIsUsingFakeTimers()) { // flushing pending timers - // Onyx.clear() promise is resolved in batch which happends after the current microtasks cycle + // Onyx.clear() promise is resolved in batch which happens after the current microtasks cycle setImmediate(jest.runOnlyPendingTimers); } From d5ace1f07c5c9ee61518cda384074e0a54861462 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Wed, 20 Nov 2024 11:58:55 +0200 Subject: [PATCH 12/20] remove redundant code --- tests/unit/NetworkTest.ts | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index 9607487b9e93..96a7f08e92f1 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -17,7 +17,6 @@ import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; import NetworkConnection from '@src/libs/NetworkConnection'; import ONYXKEYS from '@src/ONYXKEYS'; import type {Session as OnyxSession} from '@src/types/onyx'; -import type OnyxNetwork from '@src/types/onyx/Network'; import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx'; import * as TestHelper from '../utils/TestHelper'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; @@ -48,17 +47,9 @@ beforeEach(() => { Network.clearProcessQueueInterval(); SequentialQueue.resetQueue(); - return Promise.all([SequentialQueue.waitForIdle(), waitForBatchedUpdates(), PersistedRequests.clear(), Onyx.clear()]) - .then(() => { - return waitForBatchedUpdates(); - }) - .then( - () => - // Wait for all async operations to complete - new Promise((resolve) => { - setTimeout(resolve, 100); - }), - ); + return Promise.all([SequentialQueue.waitForIdle(), waitForBatchedUpdates(), PersistedRequests.clear(), Onyx.clear()]).then(() => { + return waitForBatchedUpdates(); + }); }); afterEach(() => { @@ -155,7 +146,6 @@ describe('NetworkTests', () => { const defaultTimeout = 1000; let sessionState: OnyxEntry; - let networkState: OnyxEntry; // Set up listeners for session and network state changes Onyx.connect({ @@ -163,11 +153,6 @@ describe('NetworkTests', () => { callback: (val) => (sessionState = val), }); - Onyx.connect({ - key: ONYXKEYS.NETWORK, - callback: (val) => (networkState = val), - }); - // Sign in test user and wait for updates await TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN); await waitForBatchedUpdates(); @@ -186,11 +171,6 @@ describe('NetworkTests', () => { ) // Second mock: Authenticate with network check and delay .mockImplementationOnce(() => { - // Check network state immediately - if (networkState?.isOffline) { - return Promise.reject(new Error('Network request failed')); - } - // create a delayed response. Timeout will expire after the second App.reconnectApp(); return new Promise((_, reject) => { setTimeout(() => { From c17d32ee29f2b1fdc444bbcf5940990867053d4a Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Wed, 20 Nov 2024 15:50:04 +0200 Subject: [PATCH 13/20] Revert "revert RequestThrottle to be function based" This reverts commit 80d634b4a2f10c4063ea7f5002c7eedda210a12b. --- src/libs/Middleware/Reauthentication.ts | 7 ++- src/libs/Network/SequentialQueue.ts | 12 +++-- src/libs/RequestThrottle.ts | 65 +++++++++++++------------ tests/unit/APITest.ts | 9 ++-- 4 files changed, 51 insertions(+), 42 deletions(-) diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index 29df3b7f0b50..e9a176005be7 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -7,14 +7,16 @@ import * as NetworkStore from '@libs/Network/NetworkStore'; import type {RequestError} from '@libs/Network/SequentialQueue'; import NetworkConnection from '@libs/NetworkConnection'; import * as Request from '@libs/Request'; +import RequestThrottle from '@libs/RequestThrottle'; import CONST from '@src/CONST'; -import * as RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type Middleware from './types'; // We store a reference to the active authentication request so that we are only ever making one request to authenticate at a time. let isAuthenticating: Promise | null = null; +const reauthThrottle = new RequestThrottle(); + function reauthenticate(commandName?: string): Promise { if (isAuthenticating) { return isAuthenticating; @@ -44,7 +46,8 @@ function reauthenticate(commandName?: string): Promise { function retryReauthenticate(commandName?: string): Promise { return Authentication.reauthenticate(commandName).catch((error: RequestError) => { - return RequestThrottle.sleep(error, 'Authenticate') + return reauthThrottle + .sleep(error, 'Authenticate') .then(() => retryReauthenticate(commandName)) .catch(() => { NetworkStore.setIsAuthenticating(false); diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 68c1be0f32a5..2ca2b043b737 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -2,10 +2,10 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; import Log from '@libs/Log'; import * as Request from '@libs/Request'; +import RequestThrottle from '@libs/RequestThrottle'; import * as PersistedRequests from '@userActions/PersistedRequests'; import * as QueuedOnyxUpdates from '@userActions/QueuedOnyxUpdates'; import CONST from '@src/CONST'; -import * as RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type OnyxRequest from '@src/types/onyx/Request'; import type {ConflictData} from '@src/types/onyx/Request'; @@ -28,6 +28,7 @@ resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; +const requestThrottle = new RequestThrottle(); /** * Puts the queue into a paused state so that no requests will be processed @@ -99,7 +100,7 @@ function process(): Promise { Log.info('[SequentialQueue] Removing persisted request because it was processed successfully.', false, {request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - RequestThrottle.clear(); + requestThrottle.clear(); return process(); }) .catch((error: RequestError) => { @@ -108,17 +109,18 @@ function process(): Promise { if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) { Log.info("[SequentialQueue] Removing persisted request because it failed and doesn't need to be retried.", false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - RequestThrottle.clear(); + requestThrottle.clear(); return process(); } PersistedRequests.rollbackOngoingRequest(); - return RequestThrottle.sleep(error, requestToProcess.command) + return requestThrottle + .sleep(error, requestToProcess.command) .then(process) .catch(() => { Onyx.update(requestToProcess.failureData ?? []); Log.info('[SequentialQueue] Removing persisted request because it failed too many times.', false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - RequestThrottle.clear(); + requestThrottle.clear(); return process(); }); }); diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts index 3bbc82ff5b45..8a6673c22a92 100644 --- a/src/libs/RequestThrottle.ts +++ b/src/libs/RequestThrottle.ts @@ -3,41 +3,44 @@ import Log from './Log'; import type {RequestError} from './Network/SequentialQueue'; import {generateRandomInt} from './NumberUtils'; -let requestWaitTime = 0; -let requestRetryCount = 0; +class RequestThrottle { + private requestWaitTime = 0; -function clear() { - requestWaitTime = 0; - requestRetryCount = 0; - Log.info(`[RequestThrottle] in clear()`); -} + private requestRetryCount = 0; -function getRequestWaitTime() { - if (requestWaitTime) { - requestWaitTime = Math.min(requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS); - } else { - requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); + clear() { + this.requestWaitTime = 0; + this.requestRetryCount = 0; + Log.info(`[RequestThrottle] in clear()`); } - return requestWaitTime; -} -function getLastRequestWaitTime() { - return requestWaitTime; -} - -function sleep(error: RequestError, command: string): Promise { - requestRetryCount++; - return new Promise((resolve, reject) => { - if (requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { - const currentRequestWaitTime = getRequestWaitTime(); - Log.info( - `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${requestRetryCount}. Wait time: ${currentRequestWaitTime}`, - ); - setTimeout(resolve, currentRequestWaitTime); - return; + getRequestWaitTime() { + if (this.requestWaitTime) { + this.requestWaitTime = Math.min(this.requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS); + } else { + this.requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS); } - reject(); - }); + return this.requestWaitTime; + } + + getLastRequestWaitTime() { + return this.requestWaitTime; + } + + sleep(error: RequestError, command: string): Promise { + this.requestRetryCount++; + return new Promise((resolve, reject) => { + if (this.requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { + const currentRequestWaitTime = this.getRequestWaitTime(); + Log.info( + `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`, + ); + setTimeout(resolve, currentRequestWaitTime); + } else { + reject(); + } + }); + } } -export {clear, getRequestWaitTime, sleep, getLastRequestWaitTime}; +export default RequestThrottle; diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index 5b6947861dbe..bc4b650fb6e5 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -9,7 +9,7 @@ import * as MainQueue from '@src/libs/Network/MainQueue'; import * as NetworkStore from '@src/libs/Network/NetworkStore'; import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; import * as Request from '@src/libs/Request'; -import * as RequestThrottle from '@src/libs/RequestThrottle'; +import RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx'; import * as TestHelper from '../utils/TestHelper'; @@ -39,6 +39,7 @@ type XhrCalls = Array<{ }>; const originalXHR = HttpUtils.xhr; +const requestThrottle = new RequestThrottle(); beforeEach(() => { global.fetch = TestHelper.getGlobalFetchMock(); @@ -47,7 +48,7 @@ beforeEach(() => { MainQueue.clear(); HttpUtils.cancelPendingRequests(); PersistedRequests.clear(); - RequestThrottle.clear(); + requestThrottle.clear(); NetworkStore.checkRequiredData(); // Wait for any Log command to finish and Onyx to fully clear @@ -243,7 +244,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, RequestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); }); }) .then(() => { @@ -256,7 +257,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, RequestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); }).then(waitForBatchedUpdates); }) .then(() => { From 008ec5645d082aecc072a795a1cf4a001d116468 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Wed, 20 Nov 2024 17:08:15 +0200 Subject: [PATCH 14/20] use sequentialQueueRequestThrottle in APITest --- src/libs/Network/SequentialQueue.ts | 12 ++++++------ tests/unit/APITest.ts | 9 ++++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 2ca2b043b737..3b4dd7591d31 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -28,7 +28,7 @@ resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; -const requestThrottle = new RequestThrottle(); +const sequentialQueueRequestThrottle = new RequestThrottle(); /** * Puts the queue into a paused state so that no requests will be processed @@ -100,7 +100,7 @@ function process(): Promise { Log.info('[SequentialQueue] Removing persisted request because it was processed successfully.', false, {request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - requestThrottle.clear(); + sequentialQueueRequestThrottle.clear(); return process(); }) .catch((error: RequestError) => { @@ -109,18 +109,18 @@ function process(): Promise { if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) { Log.info("[SequentialQueue] Removing persisted request because it failed and doesn't need to be retried.", false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - requestThrottle.clear(); + sequentialQueueRequestThrottle.clear(); return process(); } PersistedRequests.rollbackOngoingRequest(); - return requestThrottle + return sequentialQueueRequestThrottle .sleep(error, requestToProcess.command) .then(process) .catch(() => { Onyx.update(requestToProcess.failureData ?? []); Log.info('[SequentialQueue] Removing persisted request because it failed too many times.', false, {error, request: requestToProcess}); PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess); - requestThrottle.clear(); + sequentialQueueRequestThrottle.clear(); return process(); }); }); @@ -283,5 +283,5 @@ function resetQueue(): void { resolveIsReadyPromise?.(); } -export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process, resetQueue}; +export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process, resetQueue, sequentialQueueRequestThrottle}; export type {RequestError}; diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts index bc4b650fb6e5..ced9d5e68c4b 100644 --- a/tests/unit/APITest.ts +++ b/tests/unit/APITest.ts @@ -8,8 +8,8 @@ import HttpUtils from '@src/libs/HttpUtils'; import * as MainQueue from '@src/libs/Network/MainQueue'; import * as NetworkStore from '@src/libs/Network/NetworkStore'; import * as SequentialQueue from '@src/libs/Network/SequentialQueue'; +import {sequentialQueueRequestThrottle} from '@src/libs/Network/SequentialQueue'; import * as Request from '@src/libs/Request'; -import RequestThrottle from '@src/libs/RequestThrottle'; import ONYXKEYS from '@src/ONYXKEYS'; import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx'; import * as TestHelper from '../utils/TestHelper'; @@ -39,7 +39,6 @@ type XhrCalls = Array<{ }>; const originalXHR = HttpUtils.xhr; -const requestThrottle = new RequestThrottle(); beforeEach(() => { global.fetch = TestHelper.getGlobalFetchMock(); @@ -48,7 +47,7 @@ beforeEach(() => { MainQueue.clear(); HttpUtils.cancelPendingRequests(); PersistedRequests.clear(); - requestThrottle.clear(); + sequentialQueueRequestThrottle.clear(); NetworkStore.checkRequiredData(); // Wait for any Log command to finish and Onyx to fully clear @@ -244,7 +243,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, sequentialQueueRequestThrottle.getLastRequestWaitTime()); }); }) .then(() => { @@ -257,7 +256,7 @@ describe('APITests', () => { // We let the SequentialQueue process again after its wait time return new Promise((resolve) => { - setTimeout(resolve, requestThrottle.getLastRequestWaitTime()); + setTimeout(resolve, sequentialQueueRequestThrottle.getLastRequestWaitTime()); }).then(waitForBatchedUpdates); }) .then(() => { From 146fc097e0a5e777f104d5b14865d9c1b21905aa Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 21 Nov 2024 17:45:01 +0200 Subject: [PATCH 15/20] fix reauth online test, fix RequestThrottle.clear --- src/libs/Middleware/Reauthentication.ts | 3 +- src/libs/RequestThrottle.ts | 9 +- tests/unit/NetworkTest.ts | 120 ++++++++++-------------- 3 files changed, 59 insertions(+), 73 deletions(-) diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index e9a176005be7..8f1c9a3dc9b6 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -59,6 +59,7 @@ function retryReauthenticate(commandName?: string): Promise { function resetReauthentication(): void { isAuthenticating = null; + reauthThrottle.clear(); } const Reauthentication: Middleware = (response, request, isFromSequentialQueue) => @@ -151,4 +152,4 @@ const Reauthentication: Middleware = (response, request, isFromSequentialQueue) }); export default Reauthentication; -export {reauthenticate, resetReauthentication}; +export {reauthenticate, resetReauthentication, reauthThrottle}; diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts index 8a6673c22a92..4190e3845d72 100644 --- a/src/libs/RequestThrottle.ts +++ b/src/libs/RequestThrottle.ts @@ -8,9 +8,16 @@ class RequestThrottle { private requestRetryCount = 0; + private timeoutID?: NodeJS.Timeout; + clear() { this.requestWaitTime = 0; this.requestRetryCount = 0; + if (this.timeoutID) { + Log.info(`[RequestThrottle] clearing timeoutID: ${String(this.timeoutID)}`); + clearTimeout(this.timeoutID); + this.timeoutID = undefined; + } Log.info(`[RequestThrottle] in clear()`); } @@ -35,7 +42,7 @@ class RequestThrottle { Log.info( `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`, ); - setTimeout(resolve, currentRequestWaitTime); + this.timeoutID = setTimeout(resolve, currentRequestWaitTime); } else { reject(); } diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index 96a7f08e92f1..d9771736d4de 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -56,87 +56,65 @@ afterEach(() => { NetworkStore.resetHasReadRequiredDataFromStorage(); Onyx.addDelayToConnectCallback(0); jest.clearAllMocks(); + jest.clearAllTimers(); + jest.useRealTimers(); }); describe('NetworkTests', () => { test('failing to reauthenticate should not log out user', () => { - // Given a test user login and account ID + // Use fake timers to control timing in the test + jest.useFakeTimers(); + const TEST_USER_LOGIN = 'test@testguy.com'; const TEST_USER_ACCOUNT_ID = 1; + const NEW_AUTH_TOKEN = 'qwerty12345'; - let isOffline: boolean; - + let sessionState: OnyxEntry; Onyx.connect({ - key: ONYXKEYS.NETWORK, - callback: (val) => { - isOffline = !!val?.isOffline; - }, + key: ONYXKEYS.SESSION, + callback: (val) => (sessionState = val), }); - // Given a test user login and account ID - return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN).then(() => { - expect(isOffline).toBe(false); - - // Mock fetch() so that it throws a TypeError to simulate a bad network connection - global.fetch = jest.fn().mockRejectedValue(new TypeError(CONST.ERROR.FAILED_TO_FETCH)); - - const actualXhr = HttpUtils.xhr; - - const mockedXhr = jest.fn(); - mockedXhr - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, - }), - ) - - // Fail the call to re-authenticate - .mockImplementationOnce(actualXhr) - - // The next call should still be using the old authToken - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, - }), - ) - - // Succeed the call to set a new authToken - .mockImplementationOnce(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.SUCCESS, - authToken: 'qwerty12345', - }), - ) - - // All remaining requests should succeed - .mockImplementation(() => - Promise.resolve({ - jsonCode: CONST.JSON_CODE.SUCCESS, - }), - ); - - HttpUtils.xhr = mockedXhr; - - // This should first trigger re-authentication and then a Failed to fetch - PersonalDetails.openPublicProfilePage(TEST_USER_ACCOUNT_ID); - return waitForBatchedUpdates() - .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) - .then(() => { - expect(isOffline).toBe(false); - - // Advance the network request queue by 1 second so that it can realize it's back online - jest.advanceTimersByTime(CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); - return waitForBatchedUpdates(); - }) - .then(() => { - // Then we will eventually have 1 call to OpenPublicProfilePage and 1 calls to Authenticate - const callsToOpenPublicProfilePage = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'OpenPublicProfilePage'); - const callsToAuthenticate = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'Authenticate'); - - expect(callsToOpenPublicProfilePage.length).toBe(1); - expect(callsToAuthenticate.length).toBe(1); - }); - }); + return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN) + .then(() => { + // Mock XHR with a sequence of responses: + // 1. First call fails with NOT_AUTHENTICATED + // 2. Second call fails with network error + // 3. Third call succeeds with new auth token + const mockedXhr = jest + .fn() + .mockImplementationOnce(() => + Promise.resolve({ + jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, + }), + ) + .mockImplementationOnce(() => Promise.reject(new Error(CONST.ERROR.FAILED_TO_FETCH))) + .mockImplementationOnce(() => + Promise.resolve({ + jsonCode: CONST.JSON_CODE.SUCCESS, + authToken: NEW_AUTH_TOKEN, + }), + ); + + HttpUtils.xhr = mockedXhr; + + // Trigger an API call that will cause reauthentication flow + PersonalDetails.openPublicProfilePage(TEST_USER_ACCOUNT_ID); + return waitForBatchedUpdates(); + }) + .then(() => { + // Process pending retry request + jest.runAllTimers(); + return waitForBatchedUpdates(); + }) + .then(() => { + // Verify: + // 1. We attempted to authenticate twice (first failed, retry succeeded) + // 2. The session has the new auth token (user wasn't logged out) + const callsToAuthenticate = (HttpUtils.xhr as Mock).mock.calls.filter(([command]) => command === 'Authenticate'); + expect(callsToAuthenticate.length).toBe(2); + expect(sessionState?.authToken).toBe(NEW_AUTH_TOKEN); + }); }); test('failing to reauthenticate while offline should not log out user', async () => { From dfb863c81d2fbe4a11c48f0b5ada970bbdd61a9f Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 21 Nov 2024 17:47:07 +0200 Subject: [PATCH 16/20] fix race condition in reauth offline test --- tests/unit/NetworkTest.ts | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts index d9771736d4de..2998aa0e8a25 100644 --- a/tests/unit/NetworkTest.ts +++ b/tests/unit/NetworkTest.ts @@ -121,7 +121,6 @@ describe('NetworkTests', () => { // 1. Setup Phase - Initialize test user and state variables const TEST_USER_LOGIN = 'test@testguy.com'; const TEST_USER_ACCOUNT_ID = 1; - const defaultTimeout = 1000; let sessionState: OnyxEntry; @@ -138,24 +137,23 @@ describe('NetworkTests', () => { const initialAuthToken = sessionState?.authToken; expect(initialAuthToken).toBeDefined(); + // Create a promise that we can resolve later to control the timing + let resolveAuthRequest: (value: unknown) => void = () => {}; + const pendingAuthRequest = new Promise((resolve) => { + resolveAuthRequest = resolve; + }); + // 2. Mock Setup Phase - Configure XHR mocks for the test sequence const mockedXhr = jest .fn() - // First mock: ReconnectApp will fail with NOT_AUTHENTICATED + // First call: Return NOT_AUTHENTICATED to trigger reauthentication .mockImplementationOnce(() => Promise.resolve({ jsonCode: CONST.JSON_CODE.NOT_AUTHENTICATED, }), ) - // Second mock: Authenticate with network check and delay - .mockImplementationOnce(() => { - // create a delayed response. Timeout will expire after the second App.reconnectApp(); - return new Promise((_, reject) => { - setTimeout(() => { - reject(new Error('Network request failed')); - }, defaultTimeout); - }); - }); + // Second call: Return a pending promise that we'll resolve later + .mockImplementationOnce(() => pendingAuthRequest); HttpUtils.xhr = mockedXhr; @@ -180,17 +178,15 @@ describe('NetworkTests', () => { await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}); await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}); - // Trigger another reconnect due to network change + // 7.Trigger another reconnect due to network change App.confirmReadyToOpenApp(); App.reconnectApp(); - await waitForBatchedUpdates(); - // 7. Wait and Verify - Wait for authenticate timeout and verify session - await new Promise((resolve) => { - setTimeout(resolve, defaultTimeout + 100); - }); + // 8. Now fail the pending authentication request + resolveAuthRequest(Promise.reject(new Error('Network request failed'))); + await waitForBatchedUpdates(); // Now we wait for all updates after the auth request fails - // Verify the session remained intact and wasn't cleared + // 9. Verify the session remained intact and wasn't cleared expect(sessionState?.authToken).toBe(initialAuthToken); }); From 1196f626493bc12144df713cc88c98f75305fe2e Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 21 Nov 2024 21:00:29 +0200 Subject: [PATCH 17/20] add comments to cleanup functions, fix interval type --- src/libs/Network/SequentialQueue.ts | 4 ++++ src/libs/Network/index.ts | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 3b4dd7591d31..be78fcb0338d 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -273,6 +273,10 @@ function waitForIdle(): Promise { return isReadyPromise; } +/** + * Clear any pending requests during test runs + * This is to prevent previous requests interfering with other tests + */ function resetQueue(): void { isSequentialQueueRunning = false; currentRequestPromise = null; diff --git a/src/libs/Network/index.ts b/src/libs/Network/index.ts index d91ea8c02553..4d27f75ab1a7 100644 --- a/src/libs/Network/index.ts +++ b/src/libs/Network/index.ts @@ -6,7 +6,8 @@ import pkg from '../../../package.json'; import * as MainQueue from './MainQueue'; import * as SequentialQueue from './SequentialQueue'; -let processQueueInterval: NodeJS.Timer; +// React Native uses a number for the timer id, but Web/NodeJS uses a Timeout object +let processQueueInterval: NodeJS.Timeout | number; // We must wait until the ActiveClientManager is ready so that we ensure only the "leader" tab processes any persisted requests ActiveClientManager.isReady().then(() => { @@ -16,12 +17,15 @@ ActiveClientManager.isReady().then(() => { processQueueInterval = setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); }); -// Clear interval +/** + * Clear any existing intervals during test runs + * This is to prevent previous intervals interfering with other tests + */ function clearProcessQueueInterval() { if (!processQueueInterval) { return; } - clearInterval(processQueueInterval as unknown as number); + clearInterval(processQueueInterval); } /** From 9ce47533ec94a0b8a2b9af16fa2e297c3eed4ca4 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 21 Nov 2024 21:05:27 +0200 Subject: [PATCH 18/20] add name param to RequestThrottle --- src/libs/Middleware/Reauthentication.ts | 2 +- src/libs/Network/SequentialQueue.ts | 2 +- src/libs/RequestThrottle.ts | 12 +++++++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index 8f1c9a3dc9b6..db68ab60381b 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -15,7 +15,7 @@ import type Middleware from './types'; // We store a reference to the active authentication request so that we are only ever making one request to authenticate at a time. let isAuthenticating: Promise | null = null; -const reauthThrottle = new RequestThrottle(); +const reauthThrottle = new RequestThrottle('Re-authentication'); function reauthenticate(commandName?: string): Promise { if (isAuthenticating) { diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index be78fcb0338d..5cde21bf3a31 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -28,7 +28,7 @@ resolveIsReadyPromise?.(); let isSequentialQueueRunning = false; let currentRequestPromise: Promise | null = null; let isQueuePaused = false; -const sequentialQueueRequestThrottle = new RequestThrottle(); +const sequentialQueueRequestThrottle = new RequestThrottle('SequentialQueue'); /** * Puts the queue into a paused state so that no requests will be processed diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts index 4190e3845d72..4507a7032564 100644 --- a/src/libs/RequestThrottle.ts +++ b/src/libs/RequestThrottle.ts @@ -10,15 +10,21 @@ class RequestThrottle { private timeoutID?: NodeJS.Timeout; + private name: string; + + constructor(name: string) { + this.name = name; + } + clear() { this.requestWaitTime = 0; this.requestRetryCount = 0; if (this.timeoutID) { - Log.info(`[RequestThrottle] clearing timeoutID: ${String(this.timeoutID)}`); + Log.info(`[RequestThrottle - ${this.name}] clearing timeoutID: ${String(this.timeoutID)}`); clearTimeout(this.timeoutID); this.timeoutID = undefined; } - Log.info(`[RequestThrottle] in clear()`); + Log.info(`[RequestThrottle - ${this.name}] in clear()`); } getRequestWaitTime() { @@ -40,7 +46,7 @@ class RequestThrottle { if (this.requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) { const currentRequestWaitTime = this.getRequestWaitTime(); Log.info( - `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`, + `[RequestThrottle - ${this.name}] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count: ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`, ); this.timeoutID = setTimeout(resolve, currentRequestWaitTime); } else { From bf399e6291065fd23e2f81ee0a517d43fb216639 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 21 Nov 2024 21:16:00 +0200 Subject: [PATCH 19/20] remove reauth onyx key --- src/ONYXKEYS.ts | 4 ---- src/libs/Middleware/Reauthentication.ts | 9 --------- 2 files changed, 13 deletions(-) diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 08feab508556..49dd42fa8281 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -36,9 +36,6 @@ const ONYXKEYS = { PERSISTED_REQUESTS: 'networkRequestQueue', PERSISTED_ONGOING_REQUESTS: 'networkOngoingRequestQueue', - /** The re-authentication request to be retried as needed */ - REAUTHENTICATION_REQUEST: 'reauthenticationRequest', - /** Stores current date */ CURRENT_DATE: 'currentDate', @@ -891,7 +888,6 @@ type OnyxValuesMapping = { [ONYXKEYS.IS_SIDEBAR_LOADED]: boolean; [ONYXKEYS.PERSISTED_REQUESTS]: OnyxTypes.Request[]; [ONYXKEYS.PERSISTED_ONGOING_REQUESTS]: OnyxTypes.Request; - [ONYXKEYS.REAUTHENTICATION_REQUEST]: OnyxTypes.Request; [ONYXKEYS.CURRENT_DATE]: string; [ONYXKEYS.CREDENTIALS]: OnyxTypes.Credentials; [ONYXKEYS.STASHED_CREDENTIALS]: OnyxTypes.Credentials; diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index db68ab60381b..d1f2c48da75e 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -9,7 +9,6 @@ import NetworkConnection from '@libs/NetworkConnection'; import * as Request from '@libs/Request'; import RequestThrottle from '@libs/RequestThrottle'; import CONST from '@src/CONST'; -import ONYXKEYS from '@src/ONYXKEYS'; import type Middleware from './types'; // We store a reference to the active authentication request so that we are only ever making one request to authenticate at a time. @@ -22,12 +21,6 @@ function reauthenticate(commandName?: string): Promise { return isAuthenticating; } - const reauthRequest = { - commandName, - }; - // eslint-disable-next-line rulesdir/prefer-actions-set-data - Onyx.set(ONYXKEYS.REAUTHENTICATION_REQUEST, reauthRequest); - isAuthenticating = retryReauthenticate(commandName) .then((response) => { return response; @@ -36,8 +29,6 @@ function reauthenticate(commandName?: string): Promise { throw error; }) .finally(() => { - // eslint-disable-next-line rulesdir/prefer-actions-set-data - Onyx.set(ONYXKEYS.REAUTHENTICATION_REQUEST, null); isAuthenticating = null; }); From 463fbe34c6a7b3dd5df08b72815104b5d0e18d79 Mon Sep 17 00:00:00 2001 From: Povilas Zirgulis Date: Thu, 21 Nov 2024 21:31:49 +0200 Subject: [PATCH 20/20] improve comments, remove unused import --- src/libs/Middleware/Reauthentication.ts | 5 ++++- src/libs/RequestThrottle.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts index d1f2c48da75e..9d95fa8af873 100644 --- a/src/libs/Middleware/Reauthentication.ts +++ b/src/libs/Middleware/Reauthentication.ts @@ -1,4 +1,3 @@ -import Onyx from 'react-native-onyx'; import redirectToSignIn from '@libs/actions/SignInRedirect'; import * as Authentication from '@libs/Authentication'; import Log from '@libs/Log'; @@ -48,8 +47,12 @@ function retryReauthenticate(commandName?: string): Promise { }); } +// Used in tests to reset the reauthentication state function resetReauthentication(): void { + // Resets the authentication state flag to allow new reauthentication flows to start fresh isAuthenticating = null; + + // Clears any pending reauth timeouts set by reauthThrottle.sleep() reauthThrottle.clear(); } diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts index 4507a7032564..c4589bb07afa 100644 --- a/src/libs/RequestThrottle.ts +++ b/src/libs/RequestThrottle.ts @@ -24,7 +24,7 @@ class RequestThrottle { clearTimeout(this.timeoutID); this.timeoutID = undefined; } - Log.info(`[RequestThrottle - ${this.name}] in clear()`); + Log.info(`[RequestThrottle - ${this.name}] cleared`); } getRequestWaitTime() {