From 3360e84606831259e603813ffb18447aebbb6e65 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 21:52:40 +0200 Subject: [PATCH 01/20] NetworkTests: Add test confirming failed requests are not retried --- tests/unit/NetworkTest.js | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 5e4923f8517c..a6bae8e5a8a2 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -271,3 +271,35 @@ test('retry network request if auth and credentials are not read from Onyx yet', expect(spyHttpUtilsXhr).toHaveBeenCalled(); }); }); + +test('retry network request if connection is lost while request is running', () => { + // Given a xhr mock that will fail as if network connection dropped + const xhr = jest.spyOn(HttpUtils, 'xhr') + .mockImplementationOnce(() => { + Onyx.merge(ONYXKEYS.NETWORK, {isOffline: true}); + return Promise.reject(new Error('Network request failed')); + }) + .mockResolvedValue({jsonCode: 200, fromRetriedResult: true}); + + // Given a regular "retriable" request (that is bound to fail) + const promise = Network.post('Get'); + + return waitForPromisesToResolve() + .then(() => { + // When network connection is recovered + Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); + return waitForPromisesToResolve(); + }) + .then(() => { + // Advance the network request queue by 1 second so that it can realize it's back online + jest.advanceTimersByTime(1000); + return waitForPromisesToResolve(); + }) + .then(() => { + // Then the request should be attempted again + expect(xhr).toHaveBeenCalledTimes(2); + + // And the promise should be resolved with the 2nd call that succeeded + return expect(promise).resolves.toEqual({jsonCode: 200, fromRetriedResult: true}); + }); +}); From 3974014362a73686c7d0bc9a9b45191eecfcb8ab Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 19:09:19 +0200 Subject: [PATCH 02/20] API.requestErrorHandler - retry some requests Retry requests when something unexpected caused an error in flight --- src/libs/API.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libs/API.js b/src/libs/API.js index 94b48e42b454..003b5f9d5ee0 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -191,6 +191,16 @@ Network.registerErrorHandler((queuedRequest, error) => { // Set an error state and signify we are done loading setSessionLoadingAndError(false, 'Cannot connect to server'); + // When the request should be retried add it back to the queue + // It will either get retried now, or later when we're back online + if (lodashGet(queuedRequest, 'data.shouldRetry')) { + Network.post(queuedRequest.command, queuedRequest.data, queuedRequest.type, queuedRequest.shouldUseSecure) + .then(queuedRequest.resolve) + .catch(queuedRequest.reject); + + return; + } + // Reject the queued request with an API offline error so that the original caller can handle it. queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); }); From 13716395c09cd61c3080f5f11ac42fe97f62e580 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 19:44:24 +0200 Subject: [PATCH 03/20] API extract reusable retry and shouldRetry functions handleExpiredAuthToken does not need a then and a catch block that do the same thing We don't need to call `addDefaultValuesToParameters` - `Network.post` already does that --- src/libs/API.js | 58 +++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 003b5f9d5ee0..6a2927672a36 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -62,6 +62,26 @@ function isAuthTokenRequired(command) { ], command); } +/** + * Determines are we supposed to retry the given request + * @param {Object} request + * @returns {Boolean} + */ +function shouldRetry(request) { + return lodashGet(request, 'data.shouldRetry', false); +} + +/** + * When the request should be retried add it back to the queue + * It will either get retried now, or later when we're back online + * @param {Object} request + */ +function retry(request) { + Network.post(request.command, request.data, request.type, request.shouldUseSecure) + .then(request.resolve) + .catch(request.reject); +} + /** * Adds default values to our request data * @@ -92,16 +112,14 @@ Network.registerParameterEnhancer(addDefaultValuesToParameters); * Function used to handle expired auth tokens. It re-authenticates with the API and * then replays the original request * - * @param {String} originalCommand - * @param {Object} [originalParameters] - * @param {String} [originalType] - * @returns {Promise} + * @param {Object} originalRequest */ -function handleExpiredAuthToken(originalCommand, originalParameters, originalType) { +function handleExpiredAuthToken(originalRequest) { // When the authentication process is running, and more API requests will be requeued and they will // be performed after authentication is done. if (isAuthenticating) { - return Network.post(originalCommand, originalParameters, originalType); + retry(originalRequest); + return; } // Prevent any more requests from being processed while authentication happens @@ -109,18 +127,8 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp isAuthenticating = true; // eslint-disable-next-line no-use-before-define - return reauthenticate(originalCommand) - .then(() => { - // Now that the API is authenticated, make the original request again with the new authToken - const params = addDefaultValuesToParameters(originalCommand, originalParameters); - return Network.post(originalCommand, params, originalType); - }) - .catch(() => ( - - // If the request did not succeed because of a networking issue or the server did not respond requeue the - // original request. - Network.post(originalCommand, originalParameters, originalType) - )); + reauthenticate(originalRequest.command) + .finally(() => retry(originalRequest)); } Network.registerRequestHandler((queuedRequest, finalParameters) => { @@ -159,15 +167,12 @@ Network.registerResponseHandler((queuedRequest, response) => { // There are some API requests that should not be retried when there is an auth failure like // creating and deleting logins. In those cases, they should handle the original response instead // of the new response created by handleExpiredAuthToken. - const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); - if (!shouldRetry || unableToReauthenticate) { + if (!shouldRetry(queuedRequest) || unableToReauthenticate) { queuedRequest.resolve(response); return; } - handleExpiredAuthToken(queuedRequest.command, queuedRequest.data, queuedRequest.type) - .then(queuedRequest.resolve) - .catch(queuedRequest.reject); + handleExpiredAuthToken(queuedRequest); return; } @@ -193,11 +198,8 @@ Network.registerErrorHandler((queuedRequest, error) => { // When the request should be retried add it back to the queue // It will either get retried now, or later when we're back online - if (lodashGet(queuedRequest, 'data.shouldRetry')) { - Network.post(queuedRequest.command, queuedRequest.data, queuedRequest.type, queuedRequest.shouldUseSecure) - .then(queuedRequest.resolve) - .catch(queuedRequest.reject); - + if (shouldRetry(queuedRequest)) { + retry(queuedRequest); return; } From 4029c726b3d262889eda67ee67ca877324ebc0b7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 19:44:40 +0200 Subject: [PATCH 04/20] Network: update post method docs --- src/libs/Network.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index d7d74b889bf6..b832aafdb2a8 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -261,9 +261,9 @@ function canProcessRequestImmediately(request) { * * @param {String} command * @param {*} [data] - * @param {String} [type] - * @param {Boolean} shouldUseSecure - Whether we should use the secure API - * @returns {Promise} + * @param {String} [type='post'] + * @param {Boolean} [shouldUseSecure=false] - Whether we should use the secure API + * @returns {Promise<{ jsonCode: Number }>} */ function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) { return new Promise((resolve, reject) => { From 24a91ef5a6a41a87ae3a528491881887ded4cb77 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 20:03:59 +0200 Subject: [PATCH 05/20] Fix API: don't allow more than one auth requests to run at the same time --- src/libs/API.js | 42 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 6a2927672a36..885953cf6253 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -13,7 +13,7 @@ import * as Network from './Network'; import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens'; import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError'; -let isAuthenticating; +let pendingAuthTask; let credentials; let authToken; @@ -115,17 +115,6 @@ Network.registerParameterEnhancer(addDefaultValuesToParameters); * @param {Object} originalRequest */ function handleExpiredAuthToken(originalRequest) { - // When the authentication process is running, and more API requests will be requeued and they will - // be performed after authentication is done. - if (isAuthenticating) { - retry(originalRequest); - return; - } - - // Prevent any more requests from being processed while authentication happens - Network.pauseRequestQueue(); - isAuthenticating = true; - // eslint-disable-next-line no-use-before-define reauthenticate(originalRequest.command) .finally(() => retry(originalRequest)); @@ -217,7 +206,7 @@ Network.registerErrorHandler((queuedRequest, error) => { * @param {String} [parameters.twoFactorAuthCode] * @param {String} [parameters.email] * @param {String} [parameters.authToken] - * @returns {Promise} + * @returns {Promise<{jsonCode: Number}>} */ function Authenticate(parameters) { const commandName = 'Authenticate'; @@ -229,7 +218,15 @@ function Authenticate(parameters) { 'partnerUserSecret', ], parameters, commandName); - return Network.post(commandName, { + // Don't run more than one Authentication requests at the same time + if (pendingAuthTask) { + return pendingAuthTask; + } + + // Make any other requests wait while authentication happens + Network.pauseRequestQueue(); + + pendingAuthTask = Network.post(commandName, { // When authenticating for the first time, we pass useExpensifyLogin as true so we check // for credentials for the expensify partnerID to let users Authenticate with their expensify user // and password. @@ -276,7 +273,15 @@ function Authenticate(parameters) { } } return response; + }) + .finally(() => { + // The authentication process is finished so the network can be unpaused to continue + // processing requests + pendingAuthTask = null; + Network.unpauseRequestQueue(); }); + + return pendingAuthTask; } /** @@ -304,18 +309,9 @@ function reauthenticate(command = '') { // new authToken updateSessionAuthTokens(response.authToken, response.encryptedAuthToken); authToken = response.authToken; - - // The authentication process is finished so the network can be unpaused to continue - // processing requests - isAuthenticating = false; - Network.unpauseRequestQueue(); }) .catch((error) => { - // If authentication fails, then the network can be unpaused - Network.unpauseRequestQueue(); - isAuthenticating = false; - // When a fetch() fails and the "API is offline" 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 // re-throw this error so it can be handled by callers of reauthenticate(). From 7489bfb6938c4912d6471433db06657c83e8ae40 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 20:19:45 +0200 Subject: [PATCH 06/20] Fix Session: reauthenticate pusher is still causing multiple authentication calls Since we're already managing a promise based logic we can just use a "finally" block --- src/libs/actions/Session/index.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/libs/actions/Session/index.js b/src/libs/actions/Session/index.js index afdf7e66f9e9..1e8406c16d44 100644 --- a/src/libs/actions/Session/index.js +++ b/src/libs/actions/Session/index.js @@ -424,11 +424,17 @@ function validateEmail(accountID, validateCode, password) { }); } -// It's necessary to throttle requests to reauthenticate since calling this multiple times will cause Pusher to -// reconnect each time when we only need to reconnect once. This way, if an authToken is expired and we try to +// We only need to reconnect once. If an authToken is expired, and we try to // subscribe to a bunch of channels at once we will only reauthenticate and force reconnect Pusher once. -const reauthenticatePusher = _.throttle(() => { +let isReconnectingPusher = false; +function reauthenticatePusher() { + if (isReconnectingPusher) { + return; + } + + isReconnectingPusher = true; Log.info('[Pusher] Re-authenticating and then reconnecting'); + API.reauthenticate('Push_Authenticate') .then(Pusher.reconnect) .catch(() => { @@ -436,8 +442,11 @@ const reauthenticatePusher = _.throttle(() => { '[PusherConnectionManager]', 'Unable to re-authenticate Pusher because we are offline.', ); + }) + .finally(() => { + isReconnectingPusher = false; }); -}, 5000, {trailing: false}); +} /** * @param {String} socketID From b3e3ba6d01d754965145c8437da55fbe983ecdb5 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 20:47:49 +0200 Subject: [PATCH 07/20] tests: update expired token test - request order is no longer reversed --- tests/unit/NetworkTest.js | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index a6bae8e5a8a2..1418343f460e 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -175,23 +175,21 @@ test('consecutive API calls eventually succeed when authToken is expired', () => authToken: 'qwerty12345', })) - // Get&returnValueList=personalDetailsList - .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, - personalDetailsList: TEST_PERSONAL_DETAILS, - })) - - // Get&returnValueList=account - .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, - account: TEST_ACCOUNT_DATA, - })) - - // Get&returnValueList=chatList - .mockImplementationOnce(() => Promise.resolve({ - jsonCode: 200, - chatList: TEST_CHAT_LIST, - })); + .mockImplementation((command, data) => { + const response = {jsonCode: 200}; + + // Get&returnValueList=chatList + // Get&returnValueList=personalDetailsList + // Get&returnValueList=account + switch (data.returnValueList) { + case 'chatList': response.chatList = TEST_CHAT_LIST; break; + case 'personalDetailsList': response.personalDetailsList = TEST_PERSONAL_DETAILS; break; + case 'account': response.account = TEST_ACCOUNT_DATA; break; + default: throw new Error('Unexpected XHR call'); + } + + return Promise.resolve(response); + }); // And then make 3 API requests in quick succession with an expired authToken and handle the response API.Get({returnValueList: 'chatList'}) From badccf5b2dc0c847eda43e536b4556908b57fdf1 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 20:50:36 +0200 Subject: [PATCH 08/20] tests: reduce expired token test size It's not needed to merge the data to Onyx if it only serves to update a local variable value --- tests/unit/NetworkTest.js | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 1418343f460e..7781b8468b20 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -132,22 +132,8 @@ test('consecutive API calls eventually succeed when authToken is expired', () => const TEST_CHAT_LIST = [1, 2, 3]; let chatList; - Onyx.connect({ - key: 'test_chatList', - callback: val => chatList = val, - }); - let account; - Onyx.connect({ - key: 'test_account', - callback: val => account = val, - }); - let personalDetailsList; - Onyx.connect({ - key: 'test_personalDetailsList', - callback: val => personalDetailsList = val, - }); // When we sign in return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN) @@ -194,15 +180,15 @@ test('consecutive API calls eventually succeed when authToken is expired', () => // And then make 3 API requests in quick succession with an expired authToken and handle the response API.Get({returnValueList: 'chatList'}) .then((response) => { - Onyx.merge('test_chatList', response.chatList); + chatList = response.chatList; }); API.Get({returnValueList: 'personalDetailsList'}) .then((response) => { - Onyx.merge('test_personalDetailsList', response.personalDetailsList); + personalDetailsList = response.personalDetailsList; }); API.Get({returnValueList: 'account'}) .then((response) => { - Onyx.merge('test_account', response.account); + account = response.account; }); return waitForPromisesToResolve(); From 2a2c36729ff22a67971096d3e38dd580f3295a14 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 2 Dec 2021 21:32:15 +0200 Subject: [PATCH 09/20] tests: Add API and Session tests regarding parallel calls --- tests/unit/APITest.js | 50 +++++++++++++++++++++++++++++++++++++++ tests/unit/SessionTest.js | 29 +++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 tests/unit/APITest.js create mode 100644 tests/unit/SessionTest.js diff --git a/tests/unit/APITest.js b/tests/unit/APITest.js new file mode 100644 index 000000000000..d5d3785d38c6 --- /dev/null +++ b/tests/unit/APITest.js @@ -0,0 +1,50 @@ +import _ from 'underscore'; +import * as API from '../../src/libs/API'; +import * as Network from '../../src/libs/Network'; +import HttpUtils from '../../src/libs/HttpUtils'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import CONFIG from '../../src/CONFIG'; + +Network.setIsReady(true); + +test('Authenticate should not make parallel auth requests', () => { + // We're setting up a basic case where all requests succeed + const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValue({jsonCode: 200}); + + // Given multiple auth calls happening at the same time + _.each(_.range(5), () => API.Authenticate({ + partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, + partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, + partnerUserID: 'testUserId', + partnerUserSecret: 'testUserSecret', + })); + + // Then a single auth request should be made + return waitForPromisesToResolve() + .then(() => { + expect(xhr).toHaveBeenCalledTimes(1); + }); +}); + +test('Multiple Authenticate calls should be resolved with the same value', () => { + // A mock where only the first xhr responds with 200 and all the rest 999 + const mockResponse = {jsonCode: 200}; + jest.spyOn(HttpUtils, 'xhr') + .mockResolvedValueOnce(mockResponse) + .mockResolvedValue({jsonCode: 999}); + + // Given multiple auth calls happening at the same time + const tasks = _.map(_.range(5), () => API.Authenticate({ + partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, + partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, + partnerUserID: 'testUserId', + partnerUserSecret: 'testUserSecret', + })); + + // Then they all should be resolved from the first response + return Promise.all(tasks) + .then((results) => { + expect(_.size(results)).toEqual(5); + _.each(results, response => expect(response).toBe(mockResponse)); + }); +}); diff --git a/tests/unit/SessionTest.js b/tests/unit/SessionTest.js new file mode 100644 index 000000000000..dd6d98129b50 --- /dev/null +++ b/tests/unit/SessionTest.js @@ -0,0 +1,29 @@ +import _ from 'underscore'; +import * as Network from '../../src/libs/Network'; +import * as Session from '../../src/libs/actions/Session'; +import * as Pusher from '../../src/libs/Pusher/pusher'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; + +Network.setIsReady(true); + +jest.mock('../../src/libs/API', () => ({ + __esModule: true, + reauthenticate: jest.fn() + .mockResolvedValue({jsonCode: 200}), +})); + +jest.mock('../../src/libs/Pusher/pusher', () => ({ + __esModule: true, + reconnect: jest.fn(), +})); + +test('Multiple reauthenticatePusher calls should result in a single reconnect call', () => { + // Given multiple reauthenticate calls happening at the same time + _.each(_.range(5), () => Session.reauthenticatePusher()); + + // Then a single reconnect call should be made + return waitForPromisesToResolve() + .then(() => { + expect(Pusher.reconnect).toHaveBeenCalledTimes(1); + }); +}); From 262469dfbfa70e05bf08c351e0628d961ac09779 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 3 Dec 2021 00:33:33 +0200 Subject: [PATCH 10/20] API: update docs --- src/libs/API.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 885953cf6253..c901053244cf 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -63,7 +63,7 @@ function isAuthTokenRequired(command) { } /** - * Determines are we supposed to retry the given request + * Determines if we're supposed to retry the given request * @param {Object} request * @returns {Boolean} */ @@ -72,7 +72,7 @@ function shouldRetry(request) { } /** - * When the request should be retried add it back to the queue + * Add the request back to the queue to be retried * It will either get retried now, or later when we're back online * @param {Object} request */ @@ -185,8 +185,6 @@ Network.registerErrorHandler((queuedRequest, error) => { // Set an error state and signify we are done loading setSessionLoadingAndError(false, 'Cannot connect to server'); - // When the request should be retried add it back to the queue - // It will either get retried now, or later when we're back online if (shouldRetry(queuedRequest)) { retry(queuedRequest); return; From 3045aca77e8ca597b6d33eb0002a1dbc2dedb029 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 10 Dec 2021 23:15:40 +0200 Subject: [PATCH 11/20] API: better names for shouldRetry and retry --- src/libs/API.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index c901053244cf..d1d70a808837 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -67,16 +67,16 @@ function isAuthTokenRequired(command) { * @param {Object} request * @returns {Boolean} */ -function shouldRetry(request) { +function isRequestRetriable(request) { return lodashGet(request, 'data.shouldRetry', false); } /** - * Add the request back to the queue to be retried + * Adds the request back to the queue to be retried * It will either get retried now, or later when we're back online * @param {Object} request */ -function retry(request) { +function retryRequest(request) { Network.post(request.command, request.data, request.type, request.shouldUseSecure) .then(request.resolve) .catch(request.reject); @@ -117,7 +117,7 @@ Network.registerParameterEnhancer(addDefaultValuesToParameters); function handleExpiredAuthToken(originalRequest) { // eslint-disable-next-line no-use-before-define reauthenticate(originalRequest.command) - .finally(() => retry(originalRequest)); + .finally(() => retryRequest(originalRequest)); } Network.registerRequestHandler((queuedRequest, finalParameters) => { @@ -156,7 +156,7 @@ Network.registerResponseHandler((queuedRequest, response) => { // There are some API requests that should not be retried when there is an auth failure like // creating and deleting logins. In those cases, they should handle the original response instead // of the new response created by handleExpiredAuthToken. - if (!shouldRetry(queuedRequest) || unableToReauthenticate) { + if (!isRequestRetriable(queuedRequest) || unableToReauthenticate) { queuedRequest.resolve(response); return; } @@ -185,8 +185,8 @@ Network.registerErrorHandler((queuedRequest, error) => { // Set an error state and signify we are done loading setSessionLoadingAndError(false, 'Cannot connect to server'); - if (shouldRetry(queuedRequest)) { - retry(queuedRequest); + if (isRequestRetriable(queuedRequest)) { + retryRequest(queuedRequest); return; } From b1e2a3948c5a134a60877ee13a92b5c4629a24bf Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 10 Dec 2021 23:46:58 +0200 Subject: [PATCH 12/20] Session: revert changes made to reauthenticatePusher --- src/libs/actions/Session/index.js | 17 ++++------------- tests/unit/SessionTest.js | 29 ----------------------------- 2 files changed, 4 insertions(+), 42 deletions(-) delete mode 100644 tests/unit/SessionTest.js diff --git a/src/libs/actions/Session/index.js b/src/libs/actions/Session/index.js index 1e8406c16d44..afdf7e66f9e9 100644 --- a/src/libs/actions/Session/index.js +++ b/src/libs/actions/Session/index.js @@ -424,17 +424,11 @@ function validateEmail(accountID, validateCode, password) { }); } -// We only need to reconnect once. If an authToken is expired, and we try to +// It's necessary to throttle requests to reauthenticate since calling this multiple times will cause Pusher to +// reconnect each time when we only need to reconnect once. This way, if an authToken is expired and we try to // subscribe to a bunch of channels at once we will only reauthenticate and force reconnect Pusher once. -let isReconnectingPusher = false; -function reauthenticatePusher() { - if (isReconnectingPusher) { - return; - } - - isReconnectingPusher = true; +const reauthenticatePusher = _.throttle(() => { Log.info('[Pusher] Re-authenticating and then reconnecting'); - API.reauthenticate('Push_Authenticate') .then(Pusher.reconnect) .catch(() => { @@ -442,11 +436,8 @@ function reauthenticatePusher() { '[PusherConnectionManager]', 'Unable to re-authenticate Pusher because we are offline.', ); - }) - .finally(() => { - isReconnectingPusher = false; }); -} +}, 5000, {trailing: false}); /** * @param {String} socketID diff --git a/tests/unit/SessionTest.js b/tests/unit/SessionTest.js deleted file mode 100644 index dd6d98129b50..000000000000 --- a/tests/unit/SessionTest.js +++ /dev/null @@ -1,29 +0,0 @@ -import _ from 'underscore'; -import * as Network from '../../src/libs/Network'; -import * as Session from '../../src/libs/actions/Session'; -import * as Pusher from '../../src/libs/Pusher/pusher'; -import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; - -Network.setIsReady(true); - -jest.mock('../../src/libs/API', () => ({ - __esModule: true, - reauthenticate: jest.fn() - .mockResolvedValue({jsonCode: 200}), -})); - -jest.mock('../../src/libs/Pusher/pusher', () => ({ - __esModule: true, - reconnect: jest.fn(), -})); - -test('Multiple reauthenticatePusher calls should result in a single reconnect call', () => { - // Given multiple reauthenticate calls happening at the same time - _.each(_.range(5), () => Session.reauthenticatePusher()); - - // Then a single reconnect call should be made - return waitForPromisesToResolve() - .then(() => { - expect(Pusher.reconnect).toHaveBeenCalledTimes(1); - }); -}); From a2b45ec5f4640536a137146d856985065f066ec1 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:37:13 +0200 Subject: [PATCH 13/20] Revert "Network: update post method docs" This reverts commit 4029c726b3d262889eda67ee67ca877324ebc0b7. --- src/libs/Network.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index b832aafdb2a8..d7d74b889bf6 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -261,9 +261,9 @@ function canProcessRequestImmediately(request) { * * @param {String} command * @param {*} [data] - * @param {String} [type='post'] - * @param {Boolean} [shouldUseSecure=false] - Whether we should use the secure API - * @returns {Promise<{ jsonCode: Number }>} + * @param {String} [type] + * @param {Boolean} shouldUseSecure - Whether we should use the secure API + * @returns {Promise} */ function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) { return new Promise((resolve, reject) => { From 6b5a5d1216490475c9b2189a8cdee0c570d4bcc5 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:40:05 +0200 Subject: [PATCH 14/20] Revert "Fix API: don't allow more than one auth requests to run at the same time" This reverts commit 24a91ef5a6a41a87ae3a528491881887ded4cb77. --- src/libs/API.js | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index f04caf840dce..1673d66f1170 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -13,7 +13,7 @@ import * as Network from './Network'; import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens'; import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError'; -let pendingAuthTask; +let isAuthenticating; let credentials; let authToken; @@ -115,6 +115,17 @@ Network.registerParameterEnhancer(addDefaultValuesToParameters); * @param {Object} originalRequest */ function handleExpiredAuthToken(originalRequest) { + // When the authentication process is running, and more API requests will be requeued and they will + // be performed after authentication is done. + if (isAuthenticating) { + retry(originalRequest); + return; + } + + // Prevent any more requests from being processed while authentication happens + Network.pauseRequestQueue(); + isAuthenticating = true; + // eslint-disable-next-line no-use-before-define reauthenticate(originalRequest.command) .finally(() => retryRequest(originalRequest)); @@ -204,7 +215,7 @@ Network.registerErrorHandler((queuedRequest, error) => { * @param {String} [parameters.twoFactorAuthCode] * @param {String} [parameters.email] * @param {String} [parameters.authToken] - * @returns {Promise<{jsonCode: Number}>} + * @returns {Promise} */ function Authenticate(parameters) { const commandName = 'Authenticate'; @@ -216,15 +227,7 @@ function Authenticate(parameters) { 'partnerUserSecret', ], parameters, commandName); - // Don't run more than one Authentication requests at the same time - if (pendingAuthTask) { - return pendingAuthTask; - } - - // Make any other requests wait while authentication happens - Network.pauseRequestQueue(); - - pendingAuthTask = Network.post(commandName, { + return Network.post(commandName, { // When authenticating for the first time, we pass useExpensifyLogin as true so we check // for credentials for the expensify partnerID to let users Authenticate with their expensify user // and password. @@ -271,15 +274,7 @@ function Authenticate(parameters) { } } return response; - }) - .finally(() => { - // The authentication process is finished so the network can be unpaused to continue - // processing requests - pendingAuthTask = null; - Network.unpauseRequestQueue(); }); - - return pendingAuthTask; } /** @@ -307,9 +302,18 @@ function reauthenticate(command = '') { // new authToken updateSessionAuthTokens(response.authToken, response.encryptedAuthToken); authToken = response.authToken; + + // The authentication process is finished so the network can be unpaused to continue + // processing requests + isAuthenticating = false; + Network.unpauseRequestQueue(); }) .catch((error) => { + // If authentication fails, then the network can be unpaused + Network.unpauseRequestQueue(); + isAuthenticating = false; + // When a fetch() fails and the "API is offline" 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 // re-throw this error so it can be handled by callers of reauthenticate(). From 99e57e6f3b9a54735563be41fad81b4303763930 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:43:54 +0200 Subject: [PATCH 15/20] Revert "tests: Add API and Session tests regarding parallel calls" This reverts commit 2a2c36729ff22a67971096d3e38dd580f3295a14. --- tests/unit/APITest.js | 50 ------------------------------------------- 1 file changed, 50 deletions(-) delete mode 100644 tests/unit/APITest.js diff --git a/tests/unit/APITest.js b/tests/unit/APITest.js deleted file mode 100644 index d5d3785d38c6..000000000000 --- a/tests/unit/APITest.js +++ /dev/null @@ -1,50 +0,0 @@ -import _ from 'underscore'; -import * as API from '../../src/libs/API'; -import * as Network from '../../src/libs/Network'; -import HttpUtils from '../../src/libs/HttpUtils'; -import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; -import CONFIG from '../../src/CONFIG'; - -Network.setIsReady(true); - -test('Authenticate should not make parallel auth requests', () => { - // We're setting up a basic case where all requests succeed - const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValue({jsonCode: 200}); - - // Given multiple auth calls happening at the same time - _.each(_.range(5), () => API.Authenticate({ - partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, - partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, - partnerUserID: 'testUserId', - partnerUserSecret: 'testUserSecret', - })); - - // Then a single auth request should be made - return waitForPromisesToResolve() - .then(() => { - expect(xhr).toHaveBeenCalledTimes(1); - }); -}); - -test('Multiple Authenticate calls should be resolved with the same value', () => { - // A mock where only the first xhr responds with 200 and all the rest 999 - const mockResponse = {jsonCode: 200}; - jest.spyOn(HttpUtils, 'xhr') - .mockResolvedValueOnce(mockResponse) - .mockResolvedValue({jsonCode: 999}); - - // Given multiple auth calls happening at the same time - const tasks = _.map(_.range(5), () => API.Authenticate({ - partnerName: CONFIG.EXPENSIFY.PARTNER_NAME, - partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD, - partnerUserID: 'testUserId', - partnerUserSecret: 'testUserSecret', - })); - - // Then they all should be resolved from the first response - return Promise.all(tasks) - .then((results) => { - expect(_.size(results)).toEqual(5); - _.each(results, response => expect(response).toBe(mockResponse)); - }); -}); From 2fd5104e67b41d343dcfad3e954f35669d10e959 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:50:33 +0200 Subject: [PATCH 16/20] Revert "tests: reduce expired token test size" This reverts commit badccf5b2dc0c847eda43e536b4556908b57fdf1. --- tests/unit/NetworkTest.js | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 7781b8468b20..1418343f460e 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -132,8 +132,22 @@ test('consecutive API calls eventually succeed when authToken is expired', () => const TEST_CHAT_LIST = [1, 2, 3]; let chatList; + Onyx.connect({ + key: 'test_chatList', + callback: val => chatList = val, + }); + let account; + Onyx.connect({ + key: 'test_account', + callback: val => account = val, + }); + let personalDetailsList; + Onyx.connect({ + key: 'test_personalDetailsList', + callback: val => personalDetailsList = val, + }); // When we sign in return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN) @@ -180,15 +194,15 @@ test('consecutive API calls eventually succeed when authToken is expired', () => // And then make 3 API requests in quick succession with an expired authToken and handle the response API.Get({returnValueList: 'chatList'}) .then((response) => { - chatList = response.chatList; + Onyx.merge('test_chatList', response.chatList); }); API.Get({returnValueList: 'personalDetailsList'}) .then((response) => { - personalDetailsList = response.personalDetailsList; + Onyx.merge('test_personalDetailsList', response.personalDetailsList); }); API.Get({returnValueList: 'account'}) .then((response) => { - account = response.account; + Onyx.merge('test_account', response.account); }); return waitForPromisesToResolve(); From c3a0e5c390bec55d461a4b22c6631c8335cec9a7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:52:35 +0200 Subject: [PATCH 17/20] Use correct function name --- src/libs/API.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/API.js b/src/libs/API.js index 1673d66f1170..937ae8910b22 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -118,7 +118,7 @@ function handleExpiredAuthToken(originalRequest) { // When the authentication process is running, and more API requests will be requeued and they will // be performed after authentication is done. if (isAuthenticating) { - retry(originalRequest); + retryRequest(originalRequest); return; } From 13397eaccfa3efde16c29d159f42cb748fa2ac06 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:58:16 +0200 Subject: [PATCH 18/20] Revert "API extract reusable retry and shouldRetry functions" This reverts commit 13716395 --- src/libs/API.js | 60 ++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 937ae8910b22..4b27bd8fce83 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -62,26 +62,6 @@ function isAuthTokenRequired(command) { ], command); } -/** - * Determines if we're supposed to retry the given request - * @param {Object} request - * @returns {Boolean} - */ -function isRequestRetriable(request) { - return lodashGet(request, 'data.shouldRetry', false); -} - -/** - * Adds the request back to the queue to be retried - * It will either get retried now, or later when we're back online - * @param {Object} request - */ -function retryRequest(request) { - Network.post(request.command, request.data, request.type, request.shouldUseSecure) - .then(request.resolve) - .catch(request.reject); -} - /** * Adds default values to our request data * @@ -112,14 +92,16 @@ Network.registerParameterEnhancer(addDefaultValuesToParameters); * Function used to handle expired auth tokens. It re-authenticates with the API and * then replays the original request * - * @param {Object} originalRequest + * @param {String} originalCommand + * @param {Object} [originalParameters] + * @param {String} [originalType] + * @returns {Promise} */ -function handleExpiredAuthToken(originalRequest) { +function handleExpiredAuthToken(originalCommand, originalParameters, originalType) { // When the authentication process is running, and more API requests will be requeued and they will // be performed after authentication is done. if (isAuthenticating) { - retryRequest(originalRequest); - return; + return Network.post(originalCommand, originalParameters, originalType); } // Prevent any more requests from being processed while authentication happens @@ -127,8 +109,18 @@ function handleExpiredAuthToken(originalRequest) { isAuthenticating = true; // eslint-disable-next-line no-use-before-define - reauthenticate(originalRequest.command) - .finally(() => retryRequest(originalRequest)); + return reauthenticate(originalCommand) + .then(() => { + // Now that the API is authenticated, make the original request again with the new authToken + const params = addDefaultValuesToParameters(originalCommand, originalParameters); + return Network.post(originalCommand, params, originalType); + }) + .catch(() => ( + + // If the request did not succeed because of a networking issue or the server did not respond requeue the + // original request. + Network.post(originalCommand, originalParameters, originalType) + )); } Network.registerRequestHandler((queuedRequest, finalParameters) => { @@ -167,12 +159,15 @@ Network.registerResponseHandler((queuedRequest, response) => { // There are some API requests that should not be retried when there is an auth failure like // creating and deleting logins. In those cases, they should handle the original response instead // of the new response created by handleExpiredAuthToken. - if (!isRequestRetriable(queuedRequest) || unableToReauthenticate) { + const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); + if (!shouldRetry || unableToReauthenticate) { queuedRequest.resolve(response); return; } - handleExpiredAuthToken(queuedRequest); + handleExpiredAuthToken(queuedRequest.command, queuedRequest.data, queuedRequest.type) + .then(queuedRequest.resolve) + .catch(queuedRequest.reject); return; } @@ -196,8 +191,13 @@ Network.registerErrorHandler((queuedRequest, error) => { // Set an error state and signify we are done loading setSessionLoadingAndError(false, 'Cannot connect to server'); - if (isRequestRetriable(queuedRequest)) { - retryRequest(queuedRequest); + // When the request should be retried add it back to the queue + // It will either get retried now, or later when we're back online + if (lodashGet(queuedRequest, 'data.shouldRetry')) { + Network.post(queuedRequest.command, queuedRequest.data, queuedRequest.type, queuedRequest.shouldUseSecure) + .then(queuedRequest.resolve) + .catch(queuedRequest.reject); + return; } From 470eaaa6ebb26904c10ea2c5acdd73200f64f1ac Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 7 Jan 2022 12:59:54 +0200 Subject: [PATCH 19/20] Revert "tests: update expired token test - request order is no longer reversed" This reverts commit b3e3ba6d01d754965145c8437da55fbe983ecdb5. --- tests/unit/NetworkTest.js | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 1418343f460e..a6bae8e5a8a2 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -175,21 +175,23 @@ test('consecutive API calls eventually succeed when authToken is expired', () => authToken: 'qwerty12345', })) - .mockImplementation((command, data) => { - const response = {jsonCode: 200}; - - // Get&returnValueList=chatList - // Get&returnValueList=personalDetailsList - // Get&returnValueList=account - switch (data.returnValueList) { - case 'chatList': response.chatList = TEST_CHAT_LIST; break; - case 'personalDetailsList': response.personalDetailsList = TEST_PERSONAL_DETAILS; break; - case 'account': response.account = TEST_ACCOUNT_DATA; break; - default: throw new Error('Unexpected XHR call'); - } - - return Promise.resolve(response); - }); + // Get&returnValueList=personalDetailsList + .mockImplementationOnce(() => Promise.resolve({ + jsonCode: 200, + personalDetailsList: TEST_PERSONAL_DETAILS, + })) + + // Get&returnValueList=account + .mockImplementationOnce(() => Promise.resolve({ + jsonCode: 200, + account: TEST_ACCOUNT_DATA, + })) + + // Get&returnValueList=chatList + .mockImplementationOnce(() => Promise.resolve({ + jsonCode: 200, + chatList: TEST_CHAT_LIST, + })); // And then make 3 API requests in quick succession with an expired authToken and handle the response API.Get({returnValueList: 'chatList'}) From 294a8f1da47e0665c341f7b04e528f7c040846f4 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 12 Jan 2022 14:15:57 +0200 Subject: [PATCH 20/20] Network - move the retry on network failure logic --- src/libs/API.js | 10 ---------- src/libs/Network.js | 11 ++++++++++- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 4b27bd8fce83..e8f3309b64c8 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -191,16 +191,6 @@ Network.registerErrorHandler((queuedRequest, error) => { // Set an error state and signify we are done loading setSessionLoadingAndError(false, 'Cannot connect to server'); - // When the request should be retried add it back to the queue - // It will either get retried now, or later when we're back online - if (lodashGet(queuedRequest, 'data.shouldRetry')) { - Network.post(queuedRequest.command, queuedRequest.data, queuedRequest.type, queuedRequest.shouldUseSecure) - .then(queuedRequest.resolve) - .catch(queuedRequest.reject); - - return; - } - // Reject the queued request with an API offline error so that the original caller can handle it. queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); }); diff --git a/src/libs/Network.js b/src/libs/Network.js index d7d74b889bf6..7aa19b001732 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -225,7 +225,16 @@ function processNetworkRequestQueue() { onRequest(queuedRequest, finalParameters); HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure) .then(response => onResponse(queuedRequest, response)) - .catch(error => onError(queuedRequest, error)); + .catch((error) => { + // When the request did not reach its destination add it back the queue to be retried + const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); + if (shouldRetry) { + networkRequestQueue.push(queuedRequest); + return; + } + + onError(queuedRequest, error); + }); }); // We should clear the NETWORK_REQUEST_QUEUE when we have loaded the persisted requests & they are processed.