From 204514db0428a9f826828f3c8750e3a459081f95 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 10:16:58 -1000 Subject: [PATCH 01/10] Get rid of queue pause logics --- src/libs/API.js | 12 ++++-------- src/libs/Network/NetworkStore.js | 17 +++++++++++++++++ src/libs/Network/index.js | 23 +++-------------------- src/libs/actions/Session/index.js | 1 - 4 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 21f440c97cd3..d0828aec523e 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -13,7 +13,6 @@ import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndErr import * as NetworkStore from './Network/NetworkStore'; import enhanceParameters from './Network/enhanceParameters'; -let isAuthenticating; /** * Function used to handle expired auth tokens. It re-authenticates with the API and @@ -27,13 +26,12 @@ let isAuthenticating; 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) { + if (NetworkStore.isAuthenticating()) { return Network.post(originalCommand, originalParameters, originalType); } // Prevent any more requests from being processed while authentication happens - Network.pauseRequestQueue(); - isAuthenticating = true; + NetworkStore.setIsAuthenticating(true); // eslint-disable-next-line no-use-before-define return reauthenticate(originalCommand) @@ -233,14 +231,12 @@ function reauthenticate(command = '') { // The authentication process is finished so the network can be unpaused to continue // processing requests - isAuthenticating = false; - Network.unpauseRequestQueue(); + NetworkStore.setIsAuthenticating(false); }) .catch((error) => { // If authentication fails, then the network can be unpaused - Network.unpauseRequestQueue(); - isAuthenticating = false; + NetworkStore.setIsAuthenticating(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 diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index 3d7880b942e8..39dac394fe8f 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -7,6 +7,7 @@ let credentials; let authToken; let currentUserEmail; let networkReady = false; +let authenticating = false; /** * @param {Boolean} ready @@ -75,6 +76,20 @@ function isReady() { return networkReady; } +/** + * @param {Boolean} value + */ +function setIsAuthenticating(value) { + authenticating = value; +} + +/** + * @returns {Boolean} + */ +function isAuthenticating() { + return authenticating; +} + export { getAuthToken, setAuthToken, @@ -82,4 +97,6 @@ export { getCurrentUserEmail, isReady, setIsReady, + setIsAuthenticating, + isAuthenticating, }; diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index bccf4b4c3d6f..cf54c6b1229e 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -11,7 +11,6 @@ import * as NetworkStore from './NetworkStore'; import enhanceParameters from './enhanceParameters'; let isOffline = false; -let isQueuePaused = false; let persistedRequestsQueueRunning = false; // Queue for network requests so we don't lose actions done by the user while offline @@ -161,8 +160,8 @@ function canMakeRequest(request) { return true; } - // If the queue is paused we will not make the request right now - return !isQueuePaused; + // If we are authenticating we will not make the request right now + return !NetworkStore.isAuthenticating(); } /** @@ -179,7 +178,7 @@ function canMakeRequest(request) { */ function canRetryRequest(request) { const shouldRetry = lodashGet(request, 'data.shouldRetry'); - const logParams = {command: request.command, shouldRetry, isQueuePaused}; + const logParams = {command: request.command, shouldRetry, isAuthenticating: NetworkStore.isAuthenticating()}; const returnValueList = lodashGet(request, 'data.returnValueList'); if (returnValueList) { logParams.returnValueList = returnValueList; @@ -347,20 +346,6 @@ function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSec }); } -/** - * Prevent the network queue from being processed - */ -function pauseRequestQueue() { - isQueuePaused = true; -} - -/** - * Allow the network queue to continue to be processed - */ -function unpauseRequestQueue() { - isQueuePaused = false; -} - /** * Clear the queue and cancels all pending requests * Non-cancellable requests like Log would not be cleared @@ -372,8 +357,6 @@ function clearRequestQueue() { export { post, - pauseRequestQueue, - unpauseRequestQueue, clearRequestQueue, registerResponseHandler, registerErrorHandler, diff --git a/src/libs/actions/Session/index.js b/src/libs/actions/Session/index.js index c508ca2a72ef..bbfab305d7ea 100644 --- a/src/libs/actions/Session/index.js +++ b/src/libs/actions/Session/index.js @@ -214,7 +214,6 @@ function createTemporaryLogin(authToken, email) { autoGeneratedLogin, autoGeneratedPassword, }); - Network.unpauseRequestQueue(); return createLoginResponse; }) .catch((error) => { From 91939483199f070ea4d62e874ad504c1f4ed89ec Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 11:01:01 -1000 Subject: [PATCH 02/10] Move callbacks to Events. Start separating logic more. --- src/libs/API.js | 12 +- src/libs/Network/NetworkEvents.js | 26 ++++ src/libs/Network/NetworkStore.js | 28 ++++ src/libs/Network/PersistedRequestsQueue.js | 96 +++++++++++++ src/libs/Network/index.js | 159 ++------------------- src/libs/Network/processRequest.js | 23 +++ src/libs/NetworkConnection.js | 4 +- src/libs/actions/Session/index.js | 1 - 8 files changed, 194 insertions(+), 155 deletions(-) create mode 100644 src/libs/Network/NetworkEvents.js create mode 100644 src/libs/Network/PersistedRequestsQueue.js create mode 100644 src/libs/Network/processRequest.js diff --git a/src/libs/API.js b/src/libs/API.js index d0828aec523e..97e6ee2a4876 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -12,7 +12,7 @@ import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens'; import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError'; import * as NetworkStore from './Network/NetworkStore'; import enhanceParameters from './Network/enhanceParameters'; - +import * as NetworkEvents from './Network/NetworkEvents'; /** * Function used to handle expired auth tokens. It re-authenticates with the API and @@ -48,9 +48,9 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp )); } -Network.registerLogHandler(() => Log); +NetworkEvents.registerLogHandler(() => Log); -Network.registerRequestHandler((queuedRequest, finalParameters) => { +NetworkEvents.registerRequestHandler((queuedRequest, finalParameters) => { if (queuedRequest.command === 'Log') { return; } @@ -63,11 +63,11 @@ Network.registerRequestHandler((queuedRequest, finalParameters) => { }); }); -Network.registerRequestSkippedHandler((parameters) => { +NetworkEvents.registerRequestSkippedHandler((parameters) => { Log.hmmm('Trying to make a request when Network is not ready', parameters); }); -Network.registerResponseHandler((queuedRequest, response) => { +NetworkEvents.registerResponseHandler((queuedRequest, response) => { if (queuedRequest.command !== 'Log') { Log.info('Finished API request', false, { command: queuedRequest.command, @@ -110,7 +110,7 @@ Network.registerResponseHandler((queuedRequest, response) => { queuedRequest.resolve(response); }); -Network.registerErrorHandler((queuedRequest, error) => { +NetworkEvents.registerErrorHandler((queuedRequest, error) => { if (error.name === CONST.ERROR.REQUEST_CANCELLED) { Log.info('[API] request canceled', false, queuedRequest); return; diff --git a/src/libs/Network/NetworkEvents.js b/src/libs/Network/NetworkEvents.js new file mode 100644 index 000000000000..761f2a175018 --- /dev/null +++ b/src/libs/Network/NetworkEvents.js @@ -0,0 +1,26 @@ +import createCallback from '../createCallback'; + +const [getLogger, registerLogHandler] = createCallback(); +const [triggerConnectivityResumed, onConnectivityResumed] = createCallback(); +const [onRequest, registerRequestHandler] = createCallback(); +const [onResponse, registerResponseHandler] = createCallback(); +const [onError, registerErrorHandler] = createCallback(); +const [triggerRecheckNeeded, registerConnectionCheckCallback] = createCallback(); +const [onRequestSkipped, registerRequestSkippedHandler] = createCallback(); + +export { + registerLogHandler, + getLogger, + triggerConnectivityResumed, + onConnectivityResumed, + onRequest, + registerRequestHandler, + onResponse, + registerResponseHandler, + onError, + registerErrorHandler, + triggerRecheckNeeded, + registerConnectionCheckCallback, + registerRequestSkippedHandler, + onRequestSkipped, +}; diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index 39dac394fe8f..a99a8b19fbc3 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -2,12 +2,14 @@ import lodashGet from 'lodash/get'; import Onyx from 'react-native-onyx'; import _ from 'underscore'; import ONYXKEYS from '../../ONYXKEYS'; +import * as NetworkEvents from './NetworkEvents'; let credentials; let authToken; let currentUserEmail; let networkReady = false; let authenticating = false; +let isOffline = false; /** * @param {Boolean} ready @@ -41,6 +43,31 @@ Onyx.connect({ }, }); +// We subscribe to the online/offline status of the network to determine when we should fire off API calls +// vs queueing them for later. +Onyx.connect({ + key: ONYXKEYS.NETWORK, + callback: (network) => { + if (!network) { + return; + } + + // Client becomes online emit connectivity resumed event + if (isOffline && !network.isOffline) { + NetworkEvents.triggerConnectivityResumed(); + } + + isOffline = network.isOffline; + }, +}); + +/** + * @returns {Boolean} + */ +function getIsOffline() { + return isOffline; +} + /** * @returns {String} */ @@ -99,4 +126,5 @@ export { setIsReady, setIsAuthenticating, isAuthenticating, + getIsOffline, }; diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js new file mode 100644 index 000000000000..37356e0d2655 --- /dev/null +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -0,0 +1,96 @@ +import _ from 'underscore'; +import Onyx from 'react-native-onyx'; +import * as NetworkRequestQueue from '../actions/NetworkRequestQueue'; +import * as NetworkStore from './NetworkStore'; +import * as NetworkEvents from './NetworkEvents'; +import CONST from '../../CONST'; +import ONYXKEYS from '../../ONYXKEYS'; +import * as ActiveClientManager from '../ActiveClientManager'; +import processRequest from './processRequest'; + +let persistedRequestsQueueRunning = false; + +/** + * This method will get any persisted requests and fire them off in parallel to retry them. + * If we get any jsonCode besides 407 the request is a success. It doesn't make sense to + * continually retry things that have returned a response. However, we can retry any requests + * with known networking errors like "Failed to fetch". + * + * @returns {Promise} + */ +function process() { + const persistedRequests = NetworkRequestQueue.getPersistedRequests(); + + // This sanity check is also a recursion exit point + if (NetworkStore.getIsOffline() || _.isEmpty(persistedRequests)) { + return Promise.resolve(); + } + + const tasks = _.map(persistedRequests, request => processRequest(request) + .then((response) => { + if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { + NetworkEvents.getLogger().info('Persisted optimistic request needs authentication'); + } else { + NetworkEvents.getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.'); + } + NetworkEvents.onResponse(request, response); + NetworkRequestQueue.removeRetryableRequest(request); + }) + .catch((error) => { + // If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left + if (error.message === CONST.ERROR.FAILED_TO_FETCH) { + const retryCount = NetworkRequestQueue.incrementRetries(request); + NetworkEvents.getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); + if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { + NetworkEvents.getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message}); + NetworkRequestQueue.removeRetryableRequest(request); + } + } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { + NetworkEvents.getLogger().info('Persisted request was cancelled. Not retrying.'); + NetworkEvents.onError(request); + NetworkRequestQueue.removeRetryableRequest(request); + } else { + NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, { + command: request.command, + error: error.message, + }); + NetworkRequestQueue.removeRetryableRequest(request); + } + })); + + // Do a recursive call in case the queue is not empty after processing the current batch + return Promise.all(tasks) + .then(process); +} + +function flush() { + if (persistedRequestsQueueRunning) { + return; + } + + // NETWORK_REQUEST_QUEUE is shared across clients, thus every client/tab will have a copy + // It is very important to only process the queue from leader client otherwise requests will be duplicated. + if (!ActiveClientManager.isClientTheLeader()) { + return; + } + + persistedRequestsQueueRunning = true; + + // Ensure persistedRequests are read from storage before proceeding with the queue + const connectionId = Onyx.connect({ + key: ONYXKEYS.NETWORK_REQUEST_QUEUE, + callback: () => { + Onyx.disconnect(connectionId); + process() + .finally(() => persistedRequestsQueueRunning = false); + }, + }); +} + +// Flush the queue when the connection resumes +NetworkEvents.onConnectivityResumed(flush); + +export { + // eslint-disable-next-line import/prefer-default-export + flush, +}; diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index cf54c6b1229e..33f024a6c403 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -1,144 +1,17 @@ import _ from 'underscore'; import lodashGet from 'lodash/get'; -import Onyx from 'react-native-onyx'; import HttpUtils from '../HttpUtils'; -import ONYXKEYS from '../../ONYXKEYS'; import * as ActiveClientManager from '../ActiveClientManager'; import CONST from '../../CONST'; -import createCallback from '../createCallback'; import * as NetworkRequestQueue from '../actions/NetworkRequestQueue'; import * as NetworkStore from './NetworkStore'; -import enhanceParameters from './enhanceParameters'; - -let isOffline = false; -let persistedRequestsQueueRunning = false; +import * as NetworkEvents from './NetworkEvents'; +import * as PersistedRequestsQueue from './PersistedRequestsQueue'; +import processRequest from './processRequest'; // Queue for network requests so we don't lose actions done by the user while offline let networkRequestQueue = []; -// These handlers must be registered so we can process the request, response, and errors returned from the queue. -// The first argument passed will be the queuedRequest object and the second will be either the parameters, response, or error. -const [onRequest, registerRequestHandler] = createCallback(); -const [onResponse, registerResponseHandler] = createCallback(); -const [onError, registerErrorHandler] = createCallback(); -const [onRequestSkipped, registerRequestSkippedHandler] = createCallback(); -const [getLogger, registerLogHandler] = createCallback(); -const [recheckConnectivity, registerConnectionCheckCallback] = createCallback(); - -/** - * @param {Object} request - * @param {String} request.command - * @param {Object} request.data - * @param {String} request.type - * @param {Boolean} request.shouldUseSecure - * @returns {Promise} - */ -function processRequest(request) { - const finalParameters = enhanceParameters(request.command, request.data); - - // If request is still in processing after this time, we might be offline - const timerId = setTimeout(recheckConnectivity, CONST.NETWORK.MAX_PENDING_TIME_MS); - - onRequest(request, finalParameters); - return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) - .finally(() => clearTimeout(timerId)); -} - -/** - * This method will get any persisted requests and fire them off in parallel to retry them. - * If we get any jsonCode besides 407 the request is a success. It doesn't make sense to - * continually retry things that have returned a response. However, we can retry any requests - * with known networking errors like "Failed to fetch". - * - * @returns {Promise} - */ -function processPersistedRequestsQueue() { - const persistedRequests = NetworkRequestQueue.getPersistedRequests(); - - // This sanity check is also a recursion exit point - if (isOffline || _.isEmpty(persistedRequests)) { - return Promise.resolve(); - } - - const tasks = _.map(persistedRequests, request => processRequest(request) - .then((response) => { - if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) { - getLogger().info('Persisted optimistic request needs authentication'); - } else { - getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.'); - } - onResponse(request, response); - NetworkRequestQueue.removeRetryableRequest(request); - }) - .catch((error) => { - // If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left - if (error.message === CONST.ERROR.FAILED_TO_FETCH) { - const retryCount = NetworkRequestQueue.incrementRetries(request); - getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); - if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { - getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message}); - NetworkRequestQueue.removeRetryableRequest(request); - } - } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { - getLogger().info('Persisted request was cancelled. Not retrying.'); - onError(request); - NetworkRequestQueue.removeRetryableRequest(request); - } else { - getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, { - command: request.command, - error: error.message, - }); - NetworkRequestQueue.removeRetryableRequest(request); - } - })); - - // Do a recursive call in case the queue is not empty after processing the current batch - return Promise.all(tasks) - .then(processPersistedRequestsQueue); -} - -function flushPersistedRequestsQueue() { - if (persistedRequestsQueueRunning) { - return; - } - - // NETWORK_REQUEST_QUEUE is shared across clients, thus every client/tab will have a copy - // It is very important to only process the queue from leader client otherwise requests will be duplicated. - if (!ActiveClientManager.isClientTheLeader()) { - return; - } - - persistedRequestsQueueRunning = true; - - // Ensure persistedRequests are read from storage before proceeding with the queue - const connectionId = Onyx.connect({ - key: ONYXKEYS.NETWORK_REQUEST_QUEUE, - callback: () => { - Onyx.disconnect(connectionId); - processPersistedRequestsQueue() - .finally(() => persistedRequestsQueueRunning = false); - }, - }); -} - -// We subscribe to the online/offline status of the network to determine when we should fire off API calls -// vs queueing them for later. -Onyx.connect({ - key: ONYXKEYS.NETWORK, - callback: (network) => { - if (!network) { - return; - } - - // Client becomes online, process the queue. - if (isOffline && !network.isOffline) { - flushPersistedRequestsQueue(); - } - - isOffline = network.isOffline; - }, -}); - /** * Checks to see if a request can be made. * @@ -151,7 +24,7 @@ Onyx.connect({ */ function canMakeRequest(request) { if (!NetworkStore.isReady()) { - onRequestSkipped({command: request.command, type: request.type}); + NetworkEvents.onRequestSkipped({command: request.command, type: request.type}); return false; } @@ -198,7 +71,7 @@ function canRetryRequest(request) { */ function processNetworkRequestQueue() { // NetInfo tells us whether the app is offline - if (isOffline) { + if (NetworkStore.getIsOffline()) { if (!networkRequestQueue.length) { return; } @@ -243,15 +116,15 @@ function processNetworkRequestQueue() { } processRequest(queuedRequest) - .then(response => onResponse(queuedRequest, response)) + .then(response => NetworkEvents.onResponse(queuedRequest, response)) .catch((error) => { // Cancelled requests should not be retried if (error.name === CONST.ERROR.REQUEST_CANCELLED) { - onError(queuedRequest, error); + NetworkEvents.onError(queuedRequest, error); return; } - recheckConnectivity(); + NetworkEvents.triggerRecheckNeeded(); // Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. if (error.message === CONST.ERROR.FAILED_TO_FETCH) { @@ -259,7 +132,7 @@ function processNetworkRequestQueue() { const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); if (shouldRetry) { const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest); - getLogger().info('A retryable request failed', false, { + NetworkEvents.getLogger().info('A retryable request failed', false, { retryCount, command: queuedRequest.command, error: error.message, @@ -270,12 +143,12 @@ function processNetworkRequestQueue() { return; } - getLogger().info('Request was retried too many times with no success. No more retries left'); + NetworkEvents.getLogger().info('Request was retried too many times with no success. No more retries left'); } - onError(queuedRequest, error); + NetworkEvents.onError(queuedRequest, error); } else { - getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { + NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { command: queuedRequest.command, error: error.message, }); @@ -294,7 +167,7 @@ function startDefaultQueue() { // Post any pending request after we launch the app ActiveClientManager.isReady().then(() => { - flushPersistedRequestsQueue(); + PersistedRequestsQueue.flush(); startDefaultQueue(); }); @@ -358,10 +231,4 @@ function clearRequestQueue() { export { post, clearRequestQueue, - registerResponseHandler, - registerErrorHandler, - registerRequestHandler, - registerRequestSkippedHandler, - registerLogHandler, - registerConnectionCheckCallback, }; diff --git a/src/libs/Network/processRequest.js b/src/libs/Network/processRequest.js new file mode 100644 index 000000000000..78498047de06 --- /dev/null +++ b/src/libs/Network/processRequest.js @@ -0,0 +1,23 @@ +import CONST from '../../CONST'; +import HttpUtils from '../HttpUtils'; +import enhanceParameters from './enhanceParameters'; +import * as NetworkEvents from './NetworkEvents'; + +/** + * @param {Object} request + * @param {String} request.command + * @param {Object} request.data + * @param {String} request.type + * @param {Boolean} request.shouldUseSecure + * @returns {Promise} + */ +export default function processRequest(request) { + const finalParameters = enhanceParameters(request.command, request.data); + + // If request is still in processing after this time, we might be offline + const timerId = setTimeout(() => NetworkEvents.triggerRecheckNeeded(), CONST.NETWORK.MAX_PENDING_TIME_MS); + + NetworkEvents.onRequest(request, finalParameters); + return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) + .finally(() => clearTimeout(timerId)); +} diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 4c31dc1867b1..343b0d8bfdec 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -4,7 +4,7 @@ import AppStateMonitor from './AppStateMonitor'; import promiseAllSettled from './promiseAllSettled'; import Log from './Log'; import * as NetworkActions from './actions/Network'; -import * as NetowrkLib from './Network'; +import * as NetworkEvents from './Network/NetworkEvents'; import CONFIG from '../CONFIG'; import CONST from '../CONST'; @@ -124,7 +124,7 @@ function recheckNetworkConnection() { subscribeToNetInfo(); } -NetowrkLib.registerConnectionCheckCallback(recheckNetworkConnection); +NetworkEvents.registerConnectionCheckCallback(recheckNetworkConnection); export default { setOfflineStatus, diff --git a/src/libs/actions/Session/index.js b/src/libs/actions/Session/index.js index bbfab305d7ea..181f50a9a272 100644 --- a/src/libs/actions/Session/index.js +++ b/src/libs/actions/Session/index.js @@ -13,7 +13,6 @@ import CONST from '../../../CONST'; import Navigation from '../../Navigation/Navigation'; import ROUTES from '../../../ROUTES'; import * as Localize from '../../Localize'; -import * as Network from '../../Network'; import UnreadIndicatorUpdater from '../../UnreadIndicatorUpdater'; import Timers from '../../Timers'; import * as Pusher from '../../Pusher/pusher'; From 5d61b2d9c1c930b8c7215061a1bce4817e2fb1ca Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 13:07:26 -1000 Subject: [PATCH 03/10] Refactor retry counter --- src/ONYXKEYS.js | 3 +- src/libs/API.js | 4 - src/libs/Network/NetworkEvents.js | 14 ++- src/libs/Network/NetworkStore.js | 24 ++--- src/libs/Network/PersistedRequestsQueue.js | 16 +-- src/libs/Network/RetryCounter.js | 27 ++++++ src/libs/Network/index.js | 107 +++++++++------------ src/libs/Network/processRequest.js | 8 +- src/libs/actions/NetworkRequestQueue.js | 48 --------- src/libs/actions/PersistedRequests.js | 58 +++++++++++ tests/actions/ReimbursementAccountTest.js | 3 +- tests/unit/NetworkTest.js | 30 +++--- 12 files changed, 179 insertions(+), 163 deletions(-) create mode 100644 src/libs/Network/RetryCounter.js delete mode 100644 src/libs/actions/NetworkRequestQueue.js create mode 100644 src/libs/actions/PersistedRequests.js diff --git a/src/ONYXKEYS.js b/src/ONYXKEYS.js index e701f99ab6a1..fa357d54f081 100755 --- a/src/ONYXKEYS.js +++ b/src/ONYXKEYS.js @@ -25,7 +25,8 @@ export default { // Boolean flag set when workspace is being created IS_CREATING_WORKSPACE: 'isCreatingWorkspace', - NETWORK_REQUEST_QUEUE: 'networkRequestQueue', + // Note: These are Persisted Requests - not all requests in the main queue as the key name might lead one to believe + PERSISTED_REQUESTS: 'networkRequestQueue', // What the active route is for our navigator. Global route that determines what views to display. CURRENT_URL: 'currentURL', diff --git a/src/libs/API.js b/src/libs/API.js index 97e6ee2a4876..13448d1e5ecd 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -63,10 +63,6 @@ NetworkEvents.registerRequestHandler((queuedRequest, finalParameters) => { }); }); -NetworkEvents.registerRequestSkippedHandler((parameters) => { - Log.hmmm('Trying to make a request when Network is not ready', parameters); -}); - NetworkEvents.registerResponseHandler((queuedRequest, response) => { if (queuedRequest.command !== 'Log') { Log.info('Finished API request', false, { diff --git a/src/libs/Network/NetworkEvents.js b/src/libs/Network/NetworkEvents.js index 761f2a175018..b2aa43475415 100644 --- a/src/libs/Network/NetworkEvents.js +++ b/src/libs/Network/NetworkEvents.js @@ -1,3 +1,4 @@ +import CONST from '../../CONST'; import createCallback from '../createCallback'; const [getLogger, registerLogHandler] = createCallback(); @@ -6,7 +7,15 @@ const [onRequest, registerRequestHandler] = createCallback(); const [onResponse, registerResponseHandler] = createCallback(); const [onError, registerErrorHandler] = createCallback(); const [triggerRecheckNeeded, registerConnectionCheckCallback] = createCallback(); -const [onRequestSkipped, registerRequestSkippedHandler] = createCallback(); + +/** + * @returns {Function} cancel timer + */ +function startRequestTimeoutTimer() { + // If request is still in processing after this time, we might be offline + const timerId = setTimeout(() => triggerRecheckNeeded(), CONST.NETWORK.MAX_PENDING_TIME_MS); + return () => clearTimeout(timerId); +} export { registerLogHandler, @@ -21,6 +30,5 @@ export { registerErrorHandler, triggerRecheckNeeded, registerConnectionCheckCallback, - registerRequestSkippedHandler, - onRequestSkipped, + startRequestTimeoutTimer, }; diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index a99a8b19fbc3..fd7473978506 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -7,23 +7,23 @@ import * as NetworkEvents from './NetworkEvents'; let credentials; let authToken; let currentUserEmail; -let networkReady = false; +let requiredDataAvailable = false; let authenticating = false; let isOffline = false; /** - * @param {Boolean} ready + * @param {Boolean} val */ -function setIsReady(ready) { - networkReady = ready; +function setIsRequiredDataAvailable(val) { + requiredDataAvailable = val; } -function checkRequiredDataAndSetNetworkReady() { +function checkRequiredData() { if (_.isUndefined(authToken) || _.isUndefined(credentials)) { return; } - setIsReady(true); + setIsRequiredDataAvailable(true); } Onyx.connect({ @@ -31,7 +31,7 @@ Onyx.connect({ callback: (val) => { authToken = lodashGet(val, 'authToken', null); currentUserEmail = lodashGet(val, 'email', null); - checkRequiredDataAndSetNetworkReady(); + checkRequiredData(); }, }); @@ -39,7 +39,7 @@ Onyx.connect({ key: ONYXKEYS.CREDENTIALS, callback: (val) => { credentials = val || null; - checkRequiredDataAndSetNetworkReady(); + checkRequiredData(); }, }); @@ -99,8 +99,8 @@ function getCurrentUserEmail() { /** * @returns {Boolean} */ -function isReady() { - return networkReady; +function isRequiredDataAvailable() { + return requiredDataAvailable; } /** @@ -122,8 +122,8 @@ export { setAuthToken, getCredentials, getCurrentUserEmail, - isReady, - setIsReady, + isRequiredDataAvailable, + setIsRequiredDataAvailable, setIsAuthenticating, isAuthenticating, getIsOffline, diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js index 37356e0d2655..73b884d2d0cf 100644 --- a/src/libs/Network/PersistedRequestsQueue.js +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -1,6 +1,6 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; -import * as NetworkRequestQueue from '../actions/NetworkRequestQueue'; +import * as PersistedRequests from '../actions/PersistedRequests'; import * as NetworkStore from './NetworkStore'; import * as NetworkEvents from './NetworkEvents'; import CONST from '../../CONST'; @@ -19,7 +19,7 @@ let persistedRequestsQueueRunning = false; * @returns {Promise} */ function process() { - const persistedRequests = NetworkRequestQueue.getPersistedRequests(); + const persistedRequests = PersistedRequests.getAll(); // This sanity check is also a recursion exit point if (NetworkStore.getIsOffline() || _.isEmpty(persistedRequests)) { @@ -34,27 +34,27 @@ function process() { NetworkEvents.getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.'); } NetworkEvents.onResponse(request, response); - NetworkRequestQueue.removeRetryableRequest(request); + PersistedRequests.remove(request); }) .catch((error) => { // If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left if (error.message === CONST.ERROR.FAILED_TO_FETCH) { - const retryCount = NetworkRequestQueue.incrementRetries(request); + const retryCount = PersistedRequests.incrementRetries(request); NetworkEvents.getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { NetworkEvents.getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message}); - NetworkRequestQueue.removeRetryableRequest(request); + PersistedRequests.remove(request); } } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { NetworkEvents.getLogger().info('Persisted request was cancelled. Not retrying.'); NetworkEvents.onError(request); - NetworkRequestQueue.removeRetryableRequest(request); + PersistedRequests.remove(request); } else { NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, { command: request.command, error: error.message, }); - NetworkRequestQueue.removeRetryableRequest(request); + PersistedRequests.remove(request); } })); @@ -78,7 +78,7 @@ function flush() { // Ensure persistedRequests are read from storage before proceeding with the queue const connectionId = Onyx.connect({ - key: ONYXKEYS.NETWORK_REQUEST_QUEUE, + key: ONYXKEYS.PERSISTED_REQUESTS, callback: () => { Onyx.disconnect(connectionId); process() diff --git a/src/libs/Network/RetryCounter.js b/src/libs/Network/RetryCounter.js new file mode 100644 index 000000000000..6885046ab8a3 --- /dev/null +++ b/src/libs/Network/RetryCounter.js @@ -0,0 +1,27 @@ +export default class RetryCounter { + constructor() { + this.retryMap = new Map(); + } + + clear() { + this.retryMap.clear(); + } + + /** + * @param {Object} request + * @returns {Number} retry count + */ + incrementRetries(request) { + const current = this.retryMap.get(request) || 0; + const next = current + 1; + this.retryMap.set(request, next); + return next; + } + + /** + * @param {Object} request + */ + remove(request) { + this.retryMap.delete(request); + } +} diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index 33f024a6c403..ba6f788d33c1 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -3,7 +3,8 @@ import lodashGet from 'lodash/get'; import HttpUtils from '../HttpUtils'; import * as ActiveClientManager from '../ActiveClientManager'; import CONST from '../../CONST'; -import * as NetworkRequestQueue from '../actions/NetworkRequestQueue'; +import * as PersistedRequests from '../actions/PersistedRequests'; +import RetryCounter from './RetryCounter'; import * as NetworkStore from './NetworkStore'; import * as NetworkEvents from './NetworkEvents'; import * as PersistedRequestsQueue from './PersistedRequestsQueue'; @@ -12,6 +13,8 @@ import processRequest from './processRequest'; // Queue for network requests so we don't lose actions done by the user while offline let networkRequestQueue = []; +const mainQueueRetryCounter = new RetryCounter(); + /** * Checks to see if a request can be made. * @@ -23,74 +26,49 @@ let networkRequestQueue = []; * @return {Boolean} */ function canMakeRequest(request) { - if (!NetworkStore.isReady()) { - NetworkEvents.onRequestSkipped({command: request.command, type: request.type}); + // We must attempt to read authToken and credentials from storage before allowing any requests to happen so that any requests that + // require authToken or trigger reauthentication will succeed. + if (!NetworkStore.isRequiredDataAvailable()) { return false; } - // These requests are always made even when the queue is paused - if (request.data.forceNetworkRequest === true) { - return true; - } - - // If we are authenticating we will not make the request right now - return !NetworkStore.isAuthenticating(); + // Some requests are always made even when we are in the process of authenticating (typically because they require no authToken e.g. Log, GetAccountStatus) + // However, if we are in the process of authenticating we always want to queue requests until we are no longer authenticating. + return request.data.forceNetworkRequest === true || !NetworkStore.isAuthenticating(); } /** - * Checks to see if a request should be retried when the queue is "paused" and logs the command name + returnValueList - * to give us some limited debugging info. We don't want to log the entire request since this could lead to - * unintentional sharing of sensitive information. - * - * @param {Object} request - * @param {String} request.command - * @param {Object} request.data - * @param {Boolean} request.data.shouldRetry - * @param {String} [request.data.returnValueList] - * @return {Boolean} + * While we are offline any requests that can be persisted are removed from the main network request queue and moved to a separate map + saved to storage. */ -function canRetryRequest(request) { - const shouldRetry = lodashGet(request, 'data.shouldRetry'); - const logParams = {command: request.command, shouldRetry, isAuthenticating: NetworkStore.isAuthenticating()}; - const returnValueList = lodashGet(request, 'data.returnValueList'); - if (returnValueList) { - logParams.returnValueList = returnValueList; - } +function removeAllPersistableRequestsFromMainQueue() { + // We filter persisted requests from the normal queue so they can be processed separately + const [networkRequestQueueWithoutPersistedRequests, requestsToPersist] = _.partition(networkRequestQueue, (request) => { + const shouldRetry = lodashGet(request, 'data.shouldRetry'); + const shouldPersist = lodashGet(request, 'data.persist'); + return !shouldRetry || !shouldPersist; + }); + + networkRequestQueue = networkRequestQueueWithoutPersistedRequests; - if (!shouldRetry) { - console.debug('Skipping request that should not be re-tried: ', logParams); - } else { - console.debug('Skipping request and re-queueing: ', logParams); + if (!requestsToPersist.length) { + return; } - return shouldRetry; + // Remove any functions as they are not serializable and cannot be stored to disk + const requestsToPersistWithoutFunctions = _.map(requestsToPersist, request => _.omit(request, val => _.isFunction(val))); + PersistedRequests.save(requestsToPersistWithoutFunctions); } /** * Process the networkRequestQueue by looping through the queue and attempting to make the requests */ function processNetworkRequestQueue() { - // NetInfo tells us whether the app is offline if (NetworkStore.getIsOffline()) { if (!networkRequestQueue.length) { return; } - const retryableRequests = []; - - // If we have a request then we need to check if it can be persisted in case we close the tab while offline. - // We filter persisted requests from the normal Queue to remove duplicates - networkRequestQueue = _.reject(networkRequestQueue, (request) => { - const shouldRetry = lodashGet(request, 'data.shouldRetry'); - if (shouldRetry && request.data.persist) { - // exclude functions as they cannot be persisted - const requestToPersist = _.omit(request, val => _.isFunction(val)); - retryableRequests.push(requestToPersist); - return true; - } - }); - if (retryableRequests.length) { - NetworkRequestQueue.saveRetryableRequests(retryableRequests); - } + + removeAllPersistableRequestsFromMainQueue(); return; } @@ -100,17 +78,19 @@ function processNetworkRequestQueue() { } // Some requests should be retried and will end up here if the following conditions are met: - // - the queue is paused - // - the request does not have forceNetworkRequest === true - // - the request does not have shouldRetry === false + // - we are in the process of authenticating and the request is retryable (most are) + // - the request does not have forceNetworkRequest === true (this will trigger it to process immediately) + // - the request does not have shouldRetry === false (specified when we do not want to retry, defaults to true) const requestsToProcessOnNextRun = []; _.each(networkRequestQueue, (queuedRequest) => { - // Some requests must be allowed to run even when the queue is paused e.g. an authentication request - // that pauses the network queue while authentication happens, then unpauses it when it's done. + // Check if we can make this request at all and if we can't see if we should save it for the next run or chuck it into the ether if (!canMakeRequest(queuedRequest)) { - if (canRetryRequest(queuedRequest)) { + const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); + if (shouldRetry) { requestsToProcessOnNextRun.push(queuedRequest); + } else { + console.debug('Skipping request that should not be re-tried: ', {command: queuedRequest.command}); } return; } @@ -118,12 +98,13 @@ function processNetworkRequestQueue() { processRequest(queuedRequest) .then(response => NetworkEvents.onResponse(queuedRequest, response)) .catch((error) => { - // Cancelled requests should not be retried + // Cancelled requests are normal and can happen when a user logs out. No extra handling is needed here. if (error.name === CONST.ERROR.REQUEST_CANCELLED) { NetworkEvents.onError(queuedRequest, error); return; } + // Because we ran into an error we assume we might be offline and do a "connection" health test NetworkEvents.triggerRecheckNeeded(); // Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. @@ -131,7 +112,7 @@ function processNetworkRequestQueue() { // When the request did not reach its destination add it back the queue to be retried if we can const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); if (shouldRetry) { - const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest); + const retryCount = mainQueueRetryCounter.incrementRetries(queuedRequest); NetworkEvents.getLogger().info('A retryable request failed', false, { retryCount, command: queuedRequest.command, @@ -156,19 +137,17 @@ function processNetworkRequestQueue() { }); }); - // We clear the request queue at the end by setting the queue to retryableRequests which will either have some + // We clear the request queue at the end by setting the queue to requestsToProcessOnNextRun which will either have some // requests we want to retry or an empty array networkRequestQueue = requestsToProcessOnNextRun; } -function startDefaultQueue() { - setInterval(processNetworkRequestQueue, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); -} - -// Post any pending request after we launch the app +// We must wait until the ActiveClientManager is ready so that we ensure only the "leader" tab processes any persisted requests ActiveClientManager.isReady().then(() => { PersistedRequestsQueue.flush(); - startDefaultQueue(); + + // Start main queue and process once every n ms delay + setInterval(processNetworkRequestQueue, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); }); /** diff --git a/src/libs/Network/processRequest.js b/src/libs/Network/processRequest.js index 78498047de06..e9e879d5b156 100644 --- a/src/libs/Network/processRequest.js +++ b/src/libs/Network/processRequest.js @@ -1,4 +1,3 @@ -import CONST from '../../CONST'; import HttpUtils from '../HttpUtils'; import enhanceParameters from './enhanceParameters'; import * as NetworkEvents from './NetworkEvents'; @@ -13,11 +12,8 @@ import * as NetworkEvents from './NetworkEvents'; */ export default function processRequest(request) { const finalParameters = enhanceParameters(request.command, request.data); - - // If request is still in processing after this time, we might be offline - const timerId = setTimeout(() => NetworkEvents.triggerRecheckNeeded(), CONST.NETWORK.MAX_PENDING_TIME_MS); - + const cancelRequestTimeoutTimer = NetworkEvents.startRequestTimeoutTimer(); NetworkEvents.onRequest(request, finalParameters); return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) - .finally(() => clearTimeout(timerId)); + .finally(() => cancelRequestTimeoutTimer()); } diff --git a/src/libs/actions/NetworkRequestQueue.js b/src/libs/actions/NetworkRequestQueue.js deleted file mode 100644 index 25ff0592b589..000000000000 --- a/src/libs/actions/NetworkRequestQueue.js +++ /dev/null @@ -1,48 +0,0 @@ -import Onyx from 'react-native-onyx'; -import _ from 'underscore'; -import lodashUnionWith from 'lodash/unionWith'; -import ONYXKEYS from '../../ONYXKEYS'; - -const retryMap = new Map(); -let persistedRequests = []; - -Onyx.connect({ - key: ONYXKEYS.NETWORK_REQUEST_QUEUE, - callback: val => persistedRequests = val || [], -}); - -function clearPersistedRequests() { - Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []); - retryMap.clear(); -} - -function saveRetryableRequests(retryableRequests) { - persistedRequests = lodashUnionWith(persistedRequests, retryableRequests, _.isEqual); - Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, persistedRequests); -} - -function removeRetryableRequest(request) { - retryMap.delete(request); - persistedRequests = _.reject(persistedRequests, r => _.isEqual(r, request)); - Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, persistedRequests); -} - -function incrementRetries(request) { - const current = retryMap.get(request) || 0; - const next = current + 1; - retryMap.set(request, next); - - return next; -} - -function getPersistedRequests() { - return persistedRequests; -} - -export { - clearPersistedRequests, - saveRetryableRequests, - getPersistedRequests, - removeRetryableRequest, - incrementRetries, -}; diff --git a/src/libs/actions/PersistedRequests.js b/src/libs/actions/PersistedRequests.js new file mode 100644 index 000000000000..62ada00c3f7f --- /dev/null +++ b/src/libs/actions/PersistedRequests.js @@ -0,0 +1,58 @@ +import Onyx from 'react-native-onyx'; +import _ from 'underscore'; +import lodashUnionWith from 'lodash/unionWith'; +import ONYXKEYS from '../../ONYXKEYS'; +import RetryCounter from '../Network/RetryCounter'; + +const persistedRequestsRetryCount = new RetryCounter(); +let persistedRequests = []; + +Onyx.connect({ + key: ONYXKEYS.PERSISTED_REQUESTS, + callback: val => persistedRequests = val || [], +}); + +function clear() { + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []); + persistedRequestsRetryCount.clear(); +} + +/** + * @param {Array} requestsToPersist + */ +function save(requestsToPersist) { + persistedRequests = lodashUnionWith(persistedRequests, requestsToPersist, _.isEqual); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); +} + +/** + * @param {Object} requestToRemove + */ +function remove(requestToRemove) { + persistedRequestsRetryCount.remove(requestToRemove); + persistedRequests = _.reject(persistedRequests, persistedRequest => _.isEqual(persistedRequest, requestToRemove)); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); +} + +/** + * @returns {Array} + */ +function getAll() { + return persistedRequests; +} + +/** + * @param {Object} request + * @returns {Number} + */ +function incrementRetries(request) { + return persistedRequestsRetryCount.incrementRetries(request); +} + +export { + clear, + save, + getAll, + remove, + incrementRetries, +}; diff --git a/tests/actions/ReimbursementAccountTest.js b/tests/actions/ReimbursementAccountTest.js index c04c49e290aa..297ee0cd9e8e 100644 --- a/tests/actions/ReimbursementAccountTest.js +++ b/tests/actions/ReimbursementAccountTest.js @@ -6,7 +6,6 @@ import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import CONST from '../../src/CONST'; import BankAccount from '../../src/libs/models/BankAccount'; -import * as Network from '../../src/libs/Network'; import * as NetworkStore from '../../src/libs/Network/NetworkStore'; const TEST_BANK_ACCOUNT_ID = 1; @@ -36,7 +35,7 @@ Onyx.connect({ beforeEach(() => Onyx.clear() .then(() => { - NetworkStore.setIsReady(true); + NetworkStore.setIsRequiredDataAvailable(true); TestHelper.signInWithTestUser(); return waitForPromisesToResolve(); })); diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index c587f220b69a..87325afa0a20 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -13,7 +13,7 @@ import CONST from '../../src/CONST'; import * as Network from '../../src/libs/Network'; import * as NetworkStore from '../../src/libs/Network/NetworkStore'; import * as Session from '../../src/libs/actions/Session'; -import * as NetworkRequestQueue from '../../src/libs/actions/NetworkRequestQueue'; +import * as PersistedRequests from '../../src/libs/actions/PersistedRequests'; import Log from '../../src/libs/Log'; // Set up manual mocks for methods used in the actions so our test does not fail. @@ -33,13 +33,13 @@ const originalXHR = HttpUtils.xhr; beforeEach(() => { HttpUtils.xhr = originalXHR; - NetworkRequestQueue.clearPersistedRequests(); + PersistedRequests.clear(); Network.clearRequestQueue(); return Onyx.clear().then(waitForPromisesToResolve); }); afterEach(() => { - NetworkStore.setIsReady(false); + NetworkStore.setIsRequiredDataAvailable(false); Onyx.addDelayToConnectCallback(0); jest.clearAllMocks(); }); @@ -234,7 +234,7 @@ test('consecutive API calls eventually succeed when authToken is expired', () => test('retry network request if auth and credentials are not read from Onyx yet', () => { // In order to test an scenario where the auth token and credentials hasn't been read from storage we set - // NetworkStore.setIsReady(false) and set the session and credentials to "ready" the Network + // NetworkStore.setIsRequiredDataAvailable(false) and set the session and credentials to "ready" the Network // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; @@ -244,7 +244,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', Onyx.addDelayToConnectCallback(ONYX_DELAY_MS); // Given initial state to Network - NetworkStore.setIsReady(false); + NetworkStore.setIsRequiredDataAvailable(false); // Given an initial value to trigger an update Onyx.merge(ONYXKEYS.CREDENTIALS, {login: 'test-login'}); @@ -259,7 +259,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', Session.fetchAccountDetails(TEST_USER_LOGIN); return waitForPromisesToResolve().then(() => { // Then we expect not having the Network ready and not making an http request - expect(NetworkStore.isReady()).toBe(false); + expect(NetworkStore.isRequiredDataAvailable()).toBe(false); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // When we resolve Onyx.connect callbacks @@ -267,7 +267,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', // Then we should expect call Network.setIsReady(true) // And We should expect not making an http request yet - expect(NetworkStore.isReady()).toBe(true); + expect(NetworkStore.isRequiredDataAvailable()).toBe(true); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // When we run processNetworkRequestQueue in the setInterval of Network.js @@ -327,7 +327,7 @@ test('requests should be persisted while offline', () => { // Then `xhr` should not be used and requests should be persisted to storage expect(xhr).not.toHaveBeenCalled(); - const persisted = NetworkRequestQueue.getPersistedRequests(); + const persisted = PersistedRequests.getAll(); expect(persisted).toEqual([ expect.objectContaining({command: 'mock command', data: expect.objectContaining({param1: 'value1'})}), expect.objectContaining({command: 'mock command', data: expect.objectContaining({param3: 'value3'})}), @@ -359,7 +359,7 @@ test('requests should resume when we are online', () => { expect.arrayContaining(['mock command', expect.objectContaining({param2: 'value2'})]), ]); - const persisted = NetworkRequestQueue.getPersistedRequests(); + const persisted = PersistedRequests.getAll(); expect(persisted).toEqual([]); }); }); @@ -391,15 +391,15 @@ test('persisted request should not be cleared until a backend response occurs', .then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})) .then(() => { // Then requests should remain persisted until the xhr call is resolved - expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(2); + expect(_.size(PersistedRequests.getAll())).toEqual(2); xhrCalls[0].resolve({jsonCode: CONST.JSON_CODE.SUCCESS}); return waitForPromisesToResolve(); }) .then(waitForPromisesToResolve) .then(() => { - expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(1); - expect(NetworkRequestQueue.getPersistedRequests()).toEqual([ + expect(_.size(PersistedRequests.getAll())).toEqual(1); + expect(PersistedRequests.getAll()).toEqual([ expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})}), ]); @@ -408,8 +408,8 @@ test('persisted request should not be cleared until a backend response occurs', return waitForPromisesToResolve(); }) .then(() => { - expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(1); - expect(NetworkRequestQueue.getPersistedRequests()).toEqual([ + expect(_.size(PersistedRequests.getAll())).toEqual(1); + expect(PersistedRequests.getAll()).toEqual([ expect.objectContaining({command: 'mock command', data: expect.objectContaining({param2: 'value2'})}), ]); @@ -418,7 +418,7 @@ test('persisted request should not be cleared until a backend response occurs', return waitForPromisesToResolve(); }) .then(() => { - expect(_.size(NetworkRequestQueue.getPersistedRequests())).toEqual(0); + expect(_.size(PersistedRequests.getAll())).toEqual(0); }); }); From 0d5285c39d491da1517240447df43a8034058c7b Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 13:18:32 -1000 Subject: [PATCH 04/10] Fix canProcessImmediately confusion and add more context about why it might be used --- src/libs/Network/index.js | 64 +++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index ba6f788d33c1..37539eb2a353 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -13,6 +13,7 @@ import processRequest from './processRequest'; // Queue for network requests so we don't lose actions done by the user while offline let networkRequestQueue = []; +// Keep track of retries for any non-persisted requests const mainQueueRetryCounter = new RetryCounter(); /** @@ -37,6 +38,34 @@ function canMakeRequest(request) { return request.data.forceNetworkRequest === true || !NetworkStore.isAuthenticating(); } +/** + * @param {Object} queuedRequest + * @param {*} error + * @returns {Boolean} true if we were able to retry + */ +function retryFailedRequest(queuedRequest, error) { + // When the request did not reach its destination add it back the queue to be retried if we can + const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); + if (!shouldRetry) { + return false; + } + + const retryCount = mainQueueRetryCounter.incrementRetries(queuedRequest); + NetworkEvents.getLogger().info('A retryable request failed', false, { + retryCount, + command: queuedRequest.command, + error: error.message, + }); + + if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) { + networkRequestQueue.push(queuedRequest); + return true; + } + + NetworkEvents.getLogger().info('Request was retried too many times with no success. No more retries left'); + return false; +} + /** * While we are offline any requests that can be persisted are removed from the main network request queue and moved to a separate map + saved to storage. */ @@ -107,26 +136,13 @@ function processNetworkRequestQueue() { // Because we ran into an error we assume we might be offline and do a "connection" health test NetworkEvents.triggerRecheckNeeded(); - // Retry and request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. + // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. if (error.message === CONST.ERROR.FAILED_TO_FETCH) { - // When the request did not reach its destination add it back the queue to be retried if we can - const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry'); - if (shouldRetry) { - const retryCount = mainQueueRetryCounter.incrementRetries(queuedRequest); - NetworkEvents.getLogger().info('A retryable request failed', false, { - retryCount, - command: queuedRequest.command, - error: error.message, - }); - - if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) { - networkRequestQueue.push(queuedRequest); - return; - } - - NetworkEvents.getLogger().info('Request was retried too many times with no success. No more retries left'); + if (retryFailedRequest()) { + return; } + // We were not able to retry so pass the error to the handler in API.js NetworkEvents.onError(queuedRequest, error); } else { NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { @@ -150,14 +166,6 @@ ActiveClientManager.isReady().then(() => { setInterval(processNetworkRequestQueue, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); }); -/** - * @param {Object} request - * @returns {Boolean} - */ -function canProcessRequestImmediately(request) { - return lodashGet(request, 'data.shouldProcessImmediately', true); -} - /** * Perform a queued post request * @@ -189,7 +197,11 @@ function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSec // Add the request to a queue of actions to perform networkRequestQueue.push(request); - if (!canProcessRequestImmediately(request)) { + // This check is mainly used to prevent API commands from triggering calls to processNetworkRequestQueue from inside the context of a previous + // call to processNetworkRequestQueue() e.g. calling a Log command without this would cause the requests in networkRequestQueue to double process + // since we call Log inside processNetworkRequestQueue(). + const shouldProcessImmediately = lodashGet(request, 'data.shouldProcessImmediately', true); + if (!shouldProcessImmediately) { return; } From 4ce55d640f1ce3a1ab77a4f6c3d0994712391035 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 13:28:43 -1000 Subject: [PATCH 05/10] fix retryFailedRequest --- src/libs/Network/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index 37539eb2a353..268ceb0467d8 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -138,7 +138,7 @@ function processNetworkRequestQueue() { // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario. if (error.message === CONST.ERROR.FAILED_TO_FETCH) { - if (retryFailedRequest()) { + if (retryFailedRequest(queuedRequest, error)) { return; } From e16211f1f91de5e585dff79270f4fef538645c69 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 15:03:11 -1000 Subject: [PATCH 06/10] use really ugly name to communicate the required data reading from storage thing --- src/libs/Network/NetworkStore.js | 16 ++++++++-------- src/libs/Network/index.js | 2 +- tests/actions/ReimbursementAccountTest.js | 2 +- tests/unit/NetworkTest.js | 10 +++++----- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index 0f5caa55af47..309154742612 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -7,15 +7,15 @@ import * as NetworkEvents from './NetworkEvents'; let credentials; let authToken; let currentUserEmail; -let requiredDataAvailable = false; +let hasReadRequiredData = false; let authenticating = false; let isOffline = false; /** * @param {Boolean} val */ -function setIsRequiredDataAvailable(val) { - requiredDataAvailable = val; +function setHasReadRequiredDataFromStorage(val) { + hasReadRequiredData = val; } /** @@ -27,7 +27,7 @@ function checkRequiredData() { return; } - setIsRequiredDataAvailable(true); + setHasReadRequiredDataFromStorage(true); } Onyx.connect({ @@ -103,8 +103,8 @@ function getCurrentUserEmail() { /** * @returns {Boolean} */ -function isRequiredDataAvailable() { - return requiredDataAvailable; +function hasReadRequiredDataFromStorage() { + return hasReadRequiredData; } /** @@ -126,8 +126,8 @@ export { setAuthToken, getCredentials, getCurrentUserEmail, - isRequiredDataAvailable, - setIsRequiredDataAvailable, + hasReadRequiredDataFromStorage, + setHasReadRequiredDataFromStorage, setIsAuthenticating, isAuthenticating, getIsOffline, diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index 3f05555f79c1..c78c492eb49a 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -29,7 +29,7 @@ const mainQueueRetryCounter = new RetryCounter(); function canMakeRequest(request) { // We must attempt to read authToken and credentials from storage before allowing any requests to happen so that any requests that // require authToken or trigger reauthentication will succeed. - if (!NetworkStore.isRequiredDataAvailable()) { + if (!NetworkStore.hasReadRequiredDataFromStorage()) { return false; } diff --git a/tests/actions/ReimbursementAccountTest.js b/tests/actions/ReimbursementAccountTest.js index 297ee0cd9e8e..bb6717c466ac 100644 --- a/tests/actions/ReimbursementAccountTest.js +++ b/tests/actions/ReimbursementAccountTest.js @@ -35,7 +35,7 @@ Onyx.connect({ beforeEach(() => Onyx.clear() .then(() => { - NetworkStore.setIsRequiredDataAvailable(true); + NetworkStore.setHasReadRequiredDataFromStorage(true); TestHelper.signInWithTestUser(); return waitForPromisesToResolve(); })); diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 87325afa0a20..3a05284dda72 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -39,7 +39,7 @@ beforeEach(() => { }); afterEach(() => { - NetworkStore.setIsRequiredDataAvailable(false); + NetworkStore.setHasReadRequiredDataFromStorage(false); Onyx.addDelayToConnectCallback(0); jest.clearAllMocks(); }); @@ -234,7 +234,7 @@ test('consecutive API calls eventually succeed when authToken is expired', () => test('retry network request if auth and credentials are not read from Onyx yet', () => { // In order to test an scenario where the auth token and credentials hasn't been read from storage we set - // NetworkStore.setIsRequiredDataAvailable(false) and set the session and credentials to "ready" the Network + // NetworkStore.setHasReadRequiredDataFromStorage(false) and set the session and credentials to "ready" the Network // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; @@ -244,7 +244,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', Onyx.addDelayToConnectCallback(ONYX_DELAY_MS); // Given initial state to Network - NetworkStore.setIsRequiredDataAvailable(false); + NetworkStore.setHasReadRequiredDataFromStorage(false); // Given an initial value to trigger an update Onyx.merge(ONYXKEYS.CREDENTIALS, {login: 'test-login'}); @@ -259,7 +259,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', Session.fetchAccountDetails(TEST_USER_LOGIN); return waitForPromisesToResolve().then(() => { // Then we expect not having the Network ready and not making an http request - expect(NetworkStore.isRequiredDataAvailable()).toBe(false); + expect(NetworkStore.hasReadRequiredDataFromStorage()).toBe(false); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // When we resolve Onyx.connect callbacks @@ -267,7 +267,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', // Then we should expect call Network.setIsReady(true) // And We should expect not making an http request yet - expect(NetworkStore.isRequiredDataAvailable()).toBe(true); + expect(NetworkStore.hasReadRequiredDataFromStorage()).toBe(true); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // When we run processNetworkRequestQueue in the setInterval of Network.js From f4c1cb2accb2cd7f7a61c3375c8fea7e7589c259 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Thu, 24 Mar 2022 15:12:51 -1000 Subject: [PATCH 07/10] update comment --- src/libs/Network/PersistedRequestsQueue.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js index 73b884d2d0cf..a3ad037ed086 100644 --- a/src/libs/Network/PersistedRequestsQueue.js +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -68,7 +68,7 @@ function flush() { return; } - // NETWORK_REQUEST_QUEUE is shared across clients, thus every client/tab will have a copy + // ONYXKEYS.PERSISTED_REQUESTS is shared across clients, thus every client/tab will have a copy // It is very important to only process the queue from leader client otherwise requests will be duplicated. if (!ActiveClientManager.isClientTheLeader()) { return; From 88279fe28cd14abbd316318d1388d77deea51fcc Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 25 Mar 2022 11:25:01 -1000 Subject: [PATCH 08/10] make event names more consistent --- src/libs/API.js | 6 +++--- src/libs/Network/NetworkEvents.js | 22 +++++++++++----------- src/libs/Network/PersistedRequestsQueue.js | 4 ++-- src/libs/Network/index.js | 6 +++--- src/libs/Network/processRequest.js | 2 +- src/libs/NetworkConnection.js | 2 +- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 5575bdc237e5..2f58618bba7b 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -50,7 +50,7 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp NetworkEvents.registerLogHandler(() => Log); -NetworkEvents.registerRequestHandler((queuedRequest, finalParameters) => { +NetworkEvents.onRequestMade((queuedRequest, finalParameters) => { if (queuedRequest.command === 'Log') { return; } @@ -63,7 +63,7 @@ NetworkEvents.registerRequestHandler((queuedRequest, finalParameters) => { }); }); -NetworkEvents.registerResponseHandler((queuedRequest, response) => { +NetworkEvents.onResponse((queuedRequest, response) => { if (queuedRequest.command !== 'Log') { Log.info('Finished API request', false, { command: queuedRequest.command, @@ -108,7 +108,7 @@ NetworkEvents.registerResponseHandler((queuedRequest, response) => { queuedRequest.resolve(response); }); -NetworkEvents.registerErrorHandler((queuedRequest, error) => { +NetworkEvents.onError((queuedRequest, error) => { if (error.name === CONST.ERROR.REQUEST_CANCELLED) { Log.info('[API] request canceled', false, queuedRequest); return; diff --git a/src/libs/Network/NetworkEvents.js b/src/libs/Network/NetworkEvents.js index b2aa43475415..94afd5616d86 100644 --- a/src/libs/Network/NetworkEvents.js +++ b/src/libs/Network/NetworkEvents.js @@ -3,18 +3,18 @@ import createCallback from '../createCallback'; const [getLogger, registerLogHandler] = createCallback(); const [triggerConnectivityResumed, onConnectivityResumed] = createCallback(); -const [onRequest, registerRequestHandler] = createCallback(); -const [onResponse, registerResponseHandler] = createCallback(); -const [onError, registerErrorHandler] = createCallback(); -const [triggerRecheckNeeded, registerConnectionCheckCallback] = createCallback(); +const [triggerRequestMade, onRequestMade] = createCallback(); +const [triggerResponse, onResponse] = createCallback(); +const [triggerError, onError] = createCallback(); +const [triggerRecheckNeeded, onRecheckNeeded] = createCallback(); /** * @returns {Function} cancel timer */ function startRequestTimeoutTimer() { // If request is still in processing after this time, we might be offline - const timerId = setTimeout(() => triggerRecheckNeeded(), CONST.NETWORK.MAX_PENDING_TIME_MS); - return () => clearTimeout(timerId); + const timerID = setTimeout(triggerRecheckNeeded, CONST.NETWORK.MAX_PENDING_TIME_MS); + return () => clearTimeout(timerID); } export { @@ -22,13 +22,13 @@ export { getLogger, triggerConnectivityResumed, onConnectivityResumed, - onRequest, - registerRequestHandler, + triggerRequestMade, + onRequestMade, onResponse, - registerResponseHandler, + triggerResponse, onError, - registerErrorHandler, + triggerError, triggerRecheckNeeded, - registerConnectionCheckCallback, + onRecheckNeeded, startRequestTimeoutTimer, }; diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js index a3ad037ed086..84c5f8c1fea2 100644 --- a/src/libs/Network/PersistedRequestsQueue.js +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -33,7 +33,7 @@ function process() { } else { NetworkEvents.getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.'); } - NetworkEvents.onResponse(request, response); + NetworkEvents.triggerResponse(request, response); PersistedRequests.remove(request); }) .catch((error) => { @@ -47,7 +47,7 @@ function process() { } } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { NetworkEvents.getLogger().info('Persisted request was cancelled. Not retrying.'); - NetworkEvents.onError(request); + NetworkEvents.triggerError(request); PersistedRequests.remove(request); } else { NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, { diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index c78c492eb49a..eda553b6d313 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -125,11 +125,11 @@ function processNetworkRequestQueue() { } processRequest(queuedRequest) - .then(response => NetworkEvents.onResponse(queuedRequest, response)) + .then(response => NetworkEvents.triggerResponse(queuedRequest, response)) .catch((error) => { // Cancelled requests are normal and can happen when a user logs out. No extra handling is needed here. if (error.name === CONST.ERROR.REQUEST_CANCELLED) { - NetworkEvents.onError(queuedRequest, error); + NetworkEvents.triggerError(queuedRequest, error); return; } @@ -144,7 +144,7 @@ function processNetworkRequestQueue() { } // We were not able to retry so pass the error to the handler in API.js - NetworkEvents.onError(queuedRequest, error); + NetworkEvents.triggerError(queuedRequest, error); } else { NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { command: queuedRequest.command, diff --git a/src/libs/Network/processRequest.js b/src/libs/Network/processRequest.js index e9e879d5b156..33310da833d5 100644 --- a/src/libs/Network/processRequest.js +++ b/src/libs/Network/processRequest.js @@ -13,7 +13,7 @@ import * as NetworkEvents from './NetworkEvents'; export default function processRequest(request) { const finalParameters = enhanceParameters(request.command, request.data); const cancelRequestTimeoutTimer = NetworkEvents.startRequestTimeoutTimer(); - NetworkEvents.onRequest(request, finalParameters); + NetworkEvents.triggerRequestMade(request, finalParameters); return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) .finally(() => cancelRequestTimeoutTimer()); } diff --git a/src/libs/NetworkConnection.js b/src/libs/NetworkConnection.js index 343b0d8bfdec..a307700055f2 100644 --- a/src/libs/NetworkConnection.js +++ b/src/libs/NetworkConnection.js @@ -124,7 +124,7 @@ function recheckNetworkConnection() { subscribeToNetInfo(); } -NetworkEvents.registerConnectionCheckCallback(recheckNetworkConnection); +NetworkEvents.onRecheckNeeded(recheckNetworkConnection); export default { setOfflineStatus, From 5792e80b72721203a0d17d1c99c22a3f77dd91ec Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 25 Mar 2022 12:10:20 -1000 Subject: [PATCH 09/10] Make requested changes --- src/libs/API.js | 2 +- src/libs/Network/NetworkEvents.js | 4 ++-- src/libs/Network/NetworkStore.js | 10 +++++----- src/libs/Network/PersistedRequestsQueue.js | 16 ++++++++-------- src/libs/Network/index.js | 6 +++--- src/libs/Network/processRequest.js | 4 +++- src/libs/{Network => }/RetryCounter.js | 0 src/libs/actions/PersistedRequests.js | 2 +- 8 files changed, 23 insertions(+), 21 deletions(-) rename src/libs/{Network => }/RetryCounter.js (100%) diff --git a/src/libs/API.js b/src/libs/API.js index 2f58618bba7b..fe3f91be3eac 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -26,7 +26,7 @@ import * as NetworkEvents from './Network/NetworkEvents'; 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 (NetworkStore.isAuthenticating()) { + if (NetworkStore.getIsAuthenticating()) { return Network.post(originalCommand, originalParameters, originalType); } diff --git a/src/libs/Network/NetworkEvents.js b/src/libs/Network/NetworkEvents.js index 94afd5616d86..f034cccd36ed 100644 --- a/src/libs/Network/NetworkEvents.js +++ b/src/libs/Network/NetworkEvents.js @@ -11,7 +11,7 @@ const [triggerRecheckNeeded, onRecheckNeeded] = createCallback(); /** * @returns {Function} cancel timer */ -function startRequestTimeoutTimer() { +function startRecheckTimeoutTimer() { // If request is still in processing after this time, we might be offline const timerID = setTimeout(triggerRecheckNeeded, CONST.NETWORK.MAX_PENDING_TIME_MS); return () => clearTimeout(timerID); @@ -30,5 +30,5 @@ export { triggerError, triggerRecheckNeeded, onRecheckNeeded, - startRequestTimeoutTimer, + startRecheckTimeoutTimer, }; diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index 309154742612..c0973fa50a13 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -8,7 +8,7 @@ let credentials; let authToken; let currentUserEmail; let hasReadRequiredData = false; -let authenticating = false; +let isAuthenticating = false; let isOffline = false; /** @@ -111,14 +111,14 @@ function hasReadRequiredDataFromStorage() { * @param {Boolean} value */ function setIsAuthenticating(value) { - authenticating = value; + isAuthenticating = value; } /** * @returns {Boolean} */ -function isAuthenticating() { - return authenticating; +function getIsAuthenticating() { + return isAuthenticating; } export { @@ -129,6 +129,6 @@ export { hasReadRequiredDataFromStorage, setHasReadRequiredDataFromStorage, setIsAuthenticating, - isAuthenticating, + getIsAuthenticating, getIsOffline, }; diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js index 84c5f8c1fea2..030b14367e44 100644 --- a/src/libs/Network/PersistedRequestsQueue.js +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -8,7 +8,7 @@ import ONYXKEYS from '../../ONYXKEYS'; import * as ActiveClientManager from '../ActiveClientManager'; import processRequest from './processRequest'; -let persistedRequestsQueueRunning = false; +let isPersistedRequestsQueueRunning = false; /** * This method will get any persisted requests and fire them off in parallel to retry them. @@ -37,12 +37,12 @@ function process() { PersistedRequests.remove(request); }) .catch((error) => { - // If we are catching a known network error like "Failed to fetch" allow this request to be retried if we have retries left + // Make this request if we are catching a known network error like "Failed to fetch" and have retries left if (error.message === CONST.ERROR.FAILED_TO_FETCH) { const retryCount = PersistedRequests.incrementRetries(request); NetworkEvents.getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message}); if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) { - NetworkEvents.getLogger().info('Request failed too many times removing from storage', false, {retryCount, command: request.command, error: error.message}); + NetworkEvents.getLogger().info('Request failed too many times, removing from storage', false, {retryCount, command: request.command, error: error.message}); PersistedRequests.remove(request); } } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { @@ -64,7 +64,7 @@ function process() { } function flush() { - if (persistedRequestsQueueRunning) { + if (isPersistedRequestsQueueRunning) { return; } @@ -74,15 +74,15 @@ function flush() { return; } - persistedRequestsQueueRunning = true; + isPersistedRequestsQueueRunning = true; // Ensure persistedRequests are read from storage before proceeding with the queue - const connectionId = Onyx.connect({ + const connectionID = Onyx.connect({ key: ONYXKEYS.PERSISTED_REQUESTS, callback: () => { - Onyx.disconnect(connectionId); + Onyx.disconnect(connectionID); process() - .finally(() => persistedRequestsQueueRunning = false); + .finally(() => isPersistedRequestsQueueRunning = false); }, }); } diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index eda553b6d313..f09ba14c46d3 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -4,7 +4,7 @@ import HttpUtils from '../HttpUtils'; import * as ActiveClientManager from '../ActiveClientManager'; import CONST from '../../CONST'; import * as PersistedRequests from '../actions/PersistedRequests'; -import RetryCounter from './RetryCounter'; +import RetryCounter from '../RetryCounter'; import * as NetworkStore from './NetworkStore'; import * as NetworkEvents from './NetworkEvents'; import * as PersistedRequestsQueue from './PersistedRequestsQueue'; @@ -35,7 +35,7 @@ function canMakeRequest(request) { // Some requests are always made even when we are in the process of authenticating (typically because they require no authToken e.g. Log, GetAccountStatus) // However, if we are in the process of authenticating we always want to queue requests until we are no longer authenticating. - return request.data.forceNetworkRequest === true || !NetworkStore.isAuthenticating(); + return request.data.forceNetworkRequest === true || !NetworkStore.getIsAuthenticating(); } /** @@ -173,7 +173,7 @@ ActiveClientManager.isReady().then(() => { * @param {String} command * @param {*} [data] * @param {String} [type] - * @param {Boolean} shouldUseSecure - Whether we should use the secure API + * @param {Boolean} [shouldUseSecure] - Whether we should use the secure API * @returns {Promise} */ function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) { diff --git a/src/libs/Network/processRequest.js b/src/libs/Network/processRequest.js index 33310da833d5..414f67b61803 100644 --- a/src/libs/Network/processRequest.js +++ b/src/libs/Network/processRequest.js @@ -12,7 +12,9 @@ import * as NetworkEvents from './NetworkEvents'; */ export default function processRequest(request) { const finalParameters = enhanceParameters(request.command, request.data); - const cancelRequestTimeoutTimer = NetworkEvents.startRequestTimeoutTimer(); + + // When the request goes past a certain amount of time we trigger a re-check of the connection + const cancelRequestTimeoutTimer = NetworkEvents.startRecheckTimeoutTimer(); NetworkEvents.triggerRequestMade(request, finalParameters); return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) .finally(() => cancelRequestTimeoutTimer()); diff --git a/src/libs/Network/RetryCounter.js b/src/libs/RetryCounter.js similarity index 100% rename from src/libs/Network/RetryCounter.js rename to src/libs/RetryCounter.js diff --git a/src/libs/actions/PersistedRequests.js b/src/libs/actions/PersistedRequests.js index 62ada00c3f7f..cde903b89c21 100644 --- a/src/libs/actions/PersistedRequests.js +++ b/src/libs/actions/PersistedRequests.js @@ -2,7 +2,7 @@ import Onyx from 'react-native-onyx'; import _ from 'underscore'; import lodashUnionWith from 'lodash/unionWith'; import ONYXKEYS from '../../ONYXKEYS'; -import RetryCounter from '../Network/RetryCounter'; +import RetryCounter from '../RetryCounter'; const persistedRequestsRetryCount = new RetryCounter(); let persistedRequests = []; From 6d0576a71469af7eb85b61f3cc78b39ceaec9be9 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Fri, 25 Mar 2022 12:59:05 -1000 Subject: [PATCH 10/10] fix name --- src/libs/actions/PersistedRequests.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libs/actions/PersistedRequests.js b/src/libs/actions/PersistedRequests.js index cde903b89c21..260526c13be2 100644 --- a/src/libs/actions/PersistedRequests.js +++ b/src/libs/actions/PersistedRequests.js @@ -4,7 +4,7 @@ import lodashUnionWith from 'lodash/unionWith'; import ONYXKEYS from '../../ONYXKEYS'; import RetryCounter from '../RetryCounter'; -const persistedRequestsRetryCount = new RetryCounter(); +const persistedRequestsRetryCounter = new RetryCounter(); let persistedRequests = []; Onyx.connect({ @@ -14,7 +14,7 @@ Onyx.connect({ function clear() { Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []); - persistedRequestsRetryCount.clear(); + persistedRequestsRetryCounter.clear(); } /** @@ -29,7 +29,7 @@ function save(requestsToPersist) { * @param {Object} requestToRemove */ function remove(requestToRemove) { - persistedRequestsRetryCount.remove(requestToRemove); + persistedRequestsRetryCounter.remove(requestToRemove); persistedRequests = _.reject(persistedRequests, persistedRequest => _.isEqual(persistedRequest, requestToRemove)); Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); } @@ -46,7 +46,7 @@ function getAll() { * @returns {Number} */ function incrementRetries(request) { - return persistedRequestsRetryCount.incrementRetries(request); + return persistedRequestsRetryCounter.incrementRetries(request); } export {