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 0dd443b93792..fe3f91be3eac 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -12,8 +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'; - -let isAuthenticating; +import * as NetworkEvents from './Network/NetworkEvents'; /** * 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.getIsAuthenticating()) { 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) @@ -50,9 +48,9 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp )); } -Network.registerLogHandler(() => Log); +NetworkEvents.registerLogHandler(() => Log); -Network.registerRequestHandler((queuedRequest, finalParameters) => { +NetworkEvents.onRequestMade((queuedRequest, finalParameters) => { if (queuedRequest.command === 'Log') { return; } @@ -65,11 +63,7 @@ Network.registerRequestHandler((queuedRequest, finalParameters) => { }); }); -Network.registerRequestSkippedHandler((parameters) => { - Log.hmmm('Trying to make a request when Network is not ready', parameters); -}); - -Network.registerResponseHandler((queuedRequest, response) => { +NetworkEvents.onResponse((queuedRequest, response) => { if (queuedRequest.command !== 'Log') { Log.info('Finished API request', false, { command: queuedRequest.command, @@ -114,7 +108,7 @@ Network.registerResponseHandler((queuedRequest, response) => { queuedRequest.resolve(response); }); -Network.registerErrorHandler((queuedRequest, error) => { +NetworkEvents.onError((queuedRequest, error) => { if (error.name === CONST.ERROR.REQUEST_CANCELLED) { Log.info('[API] request canceled', false, queuedRequest); return; @@ -241,14 +235,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/NetworkEvents.js b/src/libs/Network/NetworkEvents.js new file mode 100644 index 000000000000..f034cccd36ed --- /dev/null +++ b/src/libs/Network/NetworkEvents.js @@ -0,0 +1,34 @@ +import CONST from '../../CONST'; +import createCallback from '../createCallback'; + +const [getLogger, registerLogHandler] = createCallback(); +const [triggerConnectivityResumed, onConnectivityResumed] = createCallback(); +const [triggerRequestMade, onRequestMade] = createCallback(); +const [triggerResponse, onResponse] = createCallback(); +const [triggerError, onError] = createCallback(); +const [triggerRecheckNeeded, onRecheckNeeded] = createCallback(); + +/** + * @returns {Function} cancel timer + */ +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); +} + +export { + registerLogHandler, + getLogger, + triggerConnectivityResumed, + onConnectivityResumed, + triggerRequestMade, + onRequestMade, + onResponse, + triggerResponse, + onError, + triggerError, + triggerRecheckNeeded, + onRecheckNeeded, + startRecheckTimeoutTimer, +}; diff --git a/src/libs/Network/NetworkStore.js b/src/libs/Network/NetworkStore.js index bb3725d67d0c..c0973fa50a13 100644 --- a/src/libs/Network/NetworkStore.js +++ b/src/libs/Network/NetworkStore.js @@ -2,29 +2,32 @@ 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 hasReadRequiredData = false; +let isAuthenticating = false; +let isOffline = false; /** - * @param {Boolean} ready + * @param {Boolean} val */ -function setIsReady(ready) { - networkReady = ready; +function setHasReadRequiredDataFromStorage(val) { + hasReadRequiredData = val; } /** * This is a hack to workaround the fact that Onyx may not yet have read these values from storage by the time Network starts processing requests. * If the values are undefined we haven't read them yet. If they are null or have a value then we have and the network is "ready". */ -function checkRequiredDataAndSetNetworkReady() { +function checkRequiredData() { if (_.isUndefined(authToken) || _.isUndefined(credentials)) { return; } - setIsReady(true); + setHasReadRequiredDataFromStorage(true); } Onyx.connect({ @@ -32,7 +35,7 @@ Onyx.connect({ callback: (val) => { authToken = lodashGet(val, 'authToken', null); currentUserEmail = lodashGet(val, 'email', null); - checkRequiredDataAndSetNetworkReady(); + checkRequiredData(); }, }); @@ -40,10 +43,35 @@ Onyx.connect({ key: ONYXKEYS.CREDENTIALS, callback: (val) => { credentials = val || null; - checkRequiredDataAndSetNetworkReady(); + checkRequiredData(); }, }); +// 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} */ @@ -75,8 +103,22 @@ function getCurrentUserEmail() { /** * @returns {Boolean} */ -function isReady() { - return networkReady; +function hasReadRequiredDataFromStorage() { + return hasReadRequiredData; +} + +/** + * @param {Boolean} value + */ +function setIsAuthenticating(value) { + isAuthenticating = value; +} + +/** + * @returns {Boolean} + */ +function getIsAuthenticating() { + return isAuthenticating; } export { @@ -84,6 +126,9 @@ export { setAuthToken, getCredentials, getCurrentUserEmail, - isReady, - setIsReady, + hasReadRequiredDataFromStorage, + setHasReadRequiredDataFromStorage, + setIsAuthenticating, + getIsAuthenticating, + getIsOffline, }; diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js new file mode 100644 index 000000000000..030b14367e44 --- /dev/null +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -0,0 +1,96 @@ +import _ from 'underscore'; +import Onyx from 'react-native-onyx'; +import * as PersistedRequests from '../actions/PersistedRequests'; +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 isPersistedRequestsQueueRunning = 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 = PersistedRequests.getAll(); + + // 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.triggerResponse(request, response); + PersistedRequests.remove(request); + }) + .catch((error) => { + // 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}); + PersistedRequests.remove(request); + } + } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) { + NetworkEvents.getLogger().info('Persisted request was cancelled. Not retrying.'); + NetworkEvents.triggerError(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, + }); + PersistedRequests.remove(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 (isPersistedRequestsQueueRunning) { + return; + } + + // 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; + } + + isPersistedRequestsQueueRunning = true; + + // Ensure persistedRequests are read from storage before proceeding with the queue + const connectionID = Onyx.connect({ + key: ONYXKEYS.PERSISTED_REQUESTS, + callback: () => { + Onyx.disconnect(connectionID); + process() + .finally(() => isPersistedRequestsQueueRunning = 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 f27e418dbaf9..f09ba14c46d3 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -1,144 +1,20 @@ 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 PersistedRequests from '../actions/PersistedRequests'; +import RetryCounter from '../RetryCounter'; import * as NetworkStore from './NetworkStore'; -import enhanceParameters from './enhanceParameters'; - -let isOffline = false; -let isQueuePaused = 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; - }, -}); +// Keep track of retries for any non-persisted requests +const mainQueueRetryCounter = new RetryCounter(); /** * Checks to see if a request can be made. @@ -151,74 +27,77 @@ Onyx.connect({ * @return {Boolean} */ function canMakeRequest(request) { - if (!NetworkStore.isReady()) { - 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.hasReadRequiredDataFromStorage()) { return false; } - // These requests are always made even when the queue is paused - if (request.data.forceNetworkRequest === true) { + // 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.getIsAuthenticating(); +} + +/** + * @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; } - // If the queue is paused we will not make the request right now - return !isQueuePaused; + NetworkEvents.getLogger().info('Request was retried too many times with no success. No more retries left'); + return false; } /** - * 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, isQueuePaused}; - 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; + }); - if (!shouldRetry) { - console.debug('Skipping request that should not be re-tried: ', logParams); - } else { - console.debug('Skipping request and re-queueing: ', logParams); + networkRequestQueue = networkRequestQueueWithoutPersistedRequests; + + 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 (isOffline) { + 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; } @@ -228,56 +107,46 @@ 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; } processRequest(queuedRequest) - .then(response => onResponse(queuedRequest, response)) + .then(response => NetworkEvents.triggerResponse(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) { - onError(queuedRequest, error); + NetworkEvents.triggerError(queuedRequest, error); return; } - recheckConnectivity(); + // Because we ran into an error we assume we might be offline and do a "connection" health test + NetworkEvents.triggerRecheckNeeded(); // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario // like incorrect url, bad cors headers returned by the server, DNS lookup failure etc. 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 = NetworkRequestQueue.incrementRetries(queuedRequest); - 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; - } - - getLogger().info('Request was retried too many times with no success. No more retries left'); + if (retryFailedRequest(queuedRequest, error)) { + return; } - onError(queuedRequest, error); + // We were not able to retry so pass the error to the handler in API.js + NetworkEvents.triggerError(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, }); @@ -285,28 +154,18 @@ 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(() => { - flushPersistedRequestsQueue(); - startDefaultQueue(); -}); + PersistedRequestsQueue.flush(); -/** - * @param {Object} request - * @returns {Boolean} - */ -function canProcessRequestImmediately(request) { - return lodashGet(request, 'data.shouldProcessImmediately', true); -} + // Start main queue and process once every n ms delay + setInterval(processNetworkRequestQueue, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS); +}); /** * Perform a queued post request @@ -314,7 +173,7 @@ function canProcessRequestImmediately(request) { * @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) { @@ -339,7 +198,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; } @@ -348,20 +211,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 @@ -373,13 +222,5 @@ function clearRequestQueue() { export { post, - pauseRequestQueue, - unpauseRequestQueue, 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..414f67b61803 --- /dev/null +++ b/src/libs/Network/processRequest.js @@ -0,0 +1,21 @@ +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); + + // 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/NetworkConnection.js b/src/libs/NetworkConnection.js index 4c31dc1867b1..a307700055f2 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.onRecheckNeeded(recheckNetworkConnection); export default { setOfflineStatus, diff --git a/src/libs/RetryCounter.js b/src/libs/RetryCounter.js new file mode 100644 index 000000000000..6885046ab8a3 --- /dev/null +++ b/src/libs/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/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..260526c13be2 --- /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 '../RetryCounter'; + +const persistedRequestsRetryCounter = new RetryCounter(); +let persistedRequests = []; + +Onyx.connect({ + key: ONYXKEYS.PERSISTED_REQUESTS, + callback: val => persistedRequests = val || [], +}); + +function clear() { + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []); + persistedRequestsRetryCounter.clear(); +} + +/** + * @param {Array} requestsToPersist + */ +function save(requestsToPersist) { + persistedRequests = lodashUnionWith(persistedRequests, requestsToPersist, _.isEqual); + Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests); +} + +/** + * @param {Object} requestToRemove + */ +function remove(requestToRemove) { + persistedRequestsRetryCounter.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 persistedRequestsRetryCounter.incrementRetries(request); +} + +export { + clear, + save, + getAll, + remove, + incrementRetries, +}; diff --git a/src/libs/actions/Session/index.js b/src/libs/actions/Session/index.js index bccbee55f06d..719a0a7be7dc 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'; @@ -211,7 +210,6 @@ function createTemporaryLogin(authToken, email) { autoGeneratedLogin, autoGeneratedPassword, }); - Network.unpauseRequestQueue(); return createLoginResponse; }) .catch((error) => { diff --git a/tests/actions/ReimbursementAccountTest.js b/tests/actions/ReimbursementAccountTest.js index 05db25959fcc..bb6717c466ac 100644 --- a/tests/actions/ReimbursementAccountTest.js +++ b/tests/actions/ReimbursementAccountTest.js @@ -35,7 +35,7 @@ Onyx.connect({ beforeEach(() => Onyx.clear() .then(() => { - NetworkStore.setIsReady(true); + NetworkStore.setHasReadRequiredDataFromStorage(true); TestHelper.signInWithTestUser(); return waitForPromisesToResolve(); })); diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index c587f220b69a..3a05284dda72 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.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.setIsReady(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.setIsReady(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.isReady()).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.isReady()).toBe(true); + expect(NetworkStore.hasReadRequiredDataFromStorage()).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); }); });