diff --git a/src/libs/API.js b/src/libs/API.js index fe3f91be3eac..0b7f0075cad6 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -9,7 +9,6 @@ import requireParameters from './requireParameters'; import Log from './Log'; import * as Network from './Network'; 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'; @@ -48,21 +47,10 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp )); } +// We set the logger for Network here so that we can avoid a circular dependency NetworkEvents.registerLogHandler(() => Log); -NetworkEvents.onRequestMade((queuedRequest, finalParameters) => { - if (queuedRequest.command === 'Log') { - return; - } - - Log.info('Making API request', false, { - command: queuedRequest.command, - type: queuedRequest.type, - shouldUseSecure: queuedRequest.shouldUseSecure, - rvl: finalParameters.returnValueList, - }); -}); - +// Handle response event sent by the Network NetworkEvents.onResponse((queuedRequest, response) => { if (queuedRequest.command !== 'Log') { Log.info('Finished API request', false, { @@ -100,6 +88,7 @@ NetworkEvents.onResponse((queuedRequest, response) => { return; } + // Check to see if queuedRequest has a resolve method as this could be a persisted request which had it's promise handling logic stripped if (!queuedRequest.resolve) { return; } @@ -108,28 +97,6 @@ NetworkEvents.onResponse((queuedRequest, response) => { queuedRequest.resolve(response); }); -NetworkEvents.onError((queuedRequest, error) => { - if (error.name === CONST.ERROR.REQUEST_CANCELLED) { - Log.info('[API] request canceled', false, queuedRequest); - return; - } - if (queuedRequest.command !== 'Log') { - Log.hmmm('[API] Handled error when making request', error); - } else { - console.debug('[API] There was an error in the Log API command, unable to log to server!', error); - } - - // Set an error state and signify we are done loading - setSessionLoadingAndError(false, 'Cannot connect to server'); - - // Reject the queued request with an API offline error so that the original caller can handle it - // Note: We are checking for the reject method as this could be a persisted request which had it's promise handling logic stripped - // from it when persisted to storage - if (queuedRequest.reject) { - queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); - } -}); - /** * @param {Object} parameters * @param {Boolean} [parameters.useExpensifyLogin] diff --git a/src/libs/Network/NetworkEvents.js b/src/libs/Network/NetworkEvents.js index f034cccd36ed..25bf1d2c9b18 100644 --- a/src/libs/Network/NetworkEvents.js +++ b/src/libs/Network/NetworkEvents.js @@ -1,11 +1,14 @@ import CONST from '../../CONST'; import createCallback from '../createCallback'; +/** + * This file helps bridge the Network layer with other parts of the app like API, NetworkConnection, PersistedRequestsQueue etc. + * It helps avoid circular dependencies and by setting up event triggers and subscribers. + */ + const [getLogger, registerLogHandler] = createCallback(); const [triggerConnectivityResumed, onConnectivityResumed] = createCallback(); -const [triggerRequestMade, onRequestMade] = createCallback(); const [triggerResponse, onResponse] = createCallback(); -const [triggerError, onError] = createCallback(); const [triggerRecheckNeeded, onRecheckNeeded] = createCallback(); /** @@ -22,12 +25,8 @@ export { getLogger, triggerConnectivityResumed, onConnectivityResumed, - triggerRequestMade, - onRequestMade, onResponse, triggerResponse, - onError, - triggerError, triggerRecheckNeeded, onRecheckNeeded, startRecheckTimeoutTimer, diff --git a/src/libs/Network/PersistedRequestsQueue.js b/src/libs/Network/PersistedRequestsQueue.js index 030b14367e44..3f89f2bc0bff 100644 --- a/src/libs/Network/PersistedRequestsQueue.js +++ b/src/libs/Network/PersistedRequestsQueue.js @@ -27,33 +27,11 @@ function process() { } 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, - }); + 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); } })); diff --git a/src/libs/Network/index.js b/src/libs/Network/index.js index f09ba14c46d3..c6fe983b3bb7 100644 --- a/src/libs/Network/index.js +++ b/src/libs/Network/index.js @@ -51,7 +51,7 @@ function retryFailedRequest(queuedRequest, error) { } const retryCount = mainQueueRetryCounter.incrementRetries(queuedRequest); - NetworkEvents.getLogger().info('A retryable request failed', false, { + NetworkEvents.getLogger().info('[Network] A retryable request failed', false, { retryCount, command: queuedRequest.command, error: error.message, @@ -62,7 +62,7 @@ function retryFailedRequest(queuedRequest, error) { return true; } - NetworkEvents.getLogger().info('Request was retried too many times with no success. No more retries left'); + NetworkEvents.getLogger().info('[Network] Request was retried too many times with no success. No more retries left'); return false; } @@ -125,32 +125,20 @@ function processNetworkRequestQueue() { } processRequest(queuedRequest) - .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.triggerError(queuedRequest, error); - return; - } - // Because we ran into an error we assume we might be offline and do a "connection" health test NetworkEvents.triggerRecheckNeeded(); + if (retryFailedRequest(queuedRequest, error)) { + return; + } - // 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) { - if (retryFailedRequest(queuedRequest, error)) { - return; - } - - // We were not able to retry so pass the error to the handler in API.js - NetworkEvents.triggerError(queuedRequest, error); + if (queuedRequest.command !== 'Log') { + NetworkEvents.getLogger().hmmm('[Network] Handled error when making request', error); } else { - NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { - command: queuedRequest.command, - error: error.message, - }); + console.debug('[Network] There was an error in the Log API command, unable to log to server!', error); } + + queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE)); }); }); diff --git a/src/libs/Network/processRequest.js b/src/libs/Network/processRequest.js index 414f67b61803..b3afca7ac17e 100644 --- a/src/libs/Network/processRequest.js +++ b/src/libs/Network/processRequest.js @@ -1,6 +1,27 @@ +import lodashGet from 'lodash/get'; +import CONST from '../../CONST'; import HttpUtils from '../HttpUtils'; import enhanceParameters from './enhanceParameters'; import * as NetworkEvents from './NetworkEvents'; +import * as PersistedRequests from '../actions/PersistedRequests'; + +/** + * @param {Object} request + * @param {Object} parameters + */ +function logRequestDetails(request, parameters) { + // Don't log about log or else we'd cause an infinite loop + if (request.command === 'Log') { + return; + } + + NetworkEvents.getLogger().info('Making API request', false, { + command: request.command, + type: request.type, + shouldUseSecure: request.shouldUseSecure, + rvl: parameters.returnValueList, + }); +} /** * @param {Object} request @@ -11,11 +32,43 @@ import * as NetworkEvents from './NetworkEvents'; * @returns {Promise} */ export default function processRequest(request) { + const persisted = lodashGet(request, 'data.persist', false); 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); + logRequestDetails(request, finalParameters); return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure) + .then((response) => { + if (persisted) { + PersistedRequests.remove(request); + } + NetworkEvents.triggerResponse(request, response); + return response; + }) + .catch((error) => { + if (error.message === CONST.ERROR.FAILED_TO_FETCH) { + // Throw when we get a "Failed to fetch" error so we can retry. 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. + throw error; + } + + // Cancelled requests are normal and can happen when a user logs out. No extra handling is needed here besides + // remove the request from the PersistedRequests if the request exists. + if (error.name === CONST.ERROR.REQUEST_CANCELLED) { + NetworkEvents.getLogger().info('[Network] Request canceled', false, request); + } else { + // If we get any error that is not "Failed to fetch" create GitHub issue so we can handle it. These requests will not be retried. + NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, { + command: request.command, + error: error.message, + }); + } + + // If we did not throw and we have a persisted request that was cancelled or for an unknown error remove it so it is not retried + if (persisted) { + PersistedRequests.remove(request); + } + }) .finally(() => cancelRequestTimeoutTimer()); } diff --git a/src/libs/actions/Session/setSessionLoadingAndError.js b/src/libs/actions/Session/setSessionLoadingAndError.js deleted file mode 100644 index e28b7613994c..000000000000 --- a/src/libs/actions/Session/setSessionLoadingAndError.js +++ /dev/null @@ -1,10 +0,0 @@ -import Onyx from 'react-native-onyx'; -import ONYXKEYS from '../../../ONYXKEYS'; - -/** - * @param {Boolean} loading - * @param {String} error - */ -export default function setSessionLoadingAndError(loading, error) { - Onyx.merge(ONYXKEYS.SESSION, {loading, error}); -} diff --git a/tests/unit/MigrationTest.js b/tests/unit/MigrationTest.js index 5e5f17c6a7a3..0cd808350575 100644 --- a/tests/unit/MigrationTest.js +++ b/tests/unit/MigrationTest.js @@ -63,7 +63,7 @@ describe('MoveToIndexedDb', () => { it('Should not clear non Onyx keys from localStorage', () => { // Given some Onyx and non-Onyx data exists in localStorage - localStorage.setItem(ONYXKEYS.SESSION, JSON.stringify({authToken: 'mock-token', loading: false})); + localStorage.setItem(ONYXKEYS.SESSION, JSON.stringify({authToken: 'mock-token'})); localStorage.setItem('non-onyx-item', 'MOCK'); // When the migration runs