Network: Retry requests that failed in flight

src/libs/API.js

@@ -13,7 +13,7 @@ import * as Network from './Network';
 import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens';
 import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError';
 
-let isAuthenticating;
+let pendingAuthTask;
 let credentials;
 let authToken;
 
@@ -62,6 +62,26 @@ function isAuthTokenRequired(command) {
     ], command);
 }
 
+/**
+ * Determines are we supposed to retry the given request
+ * @param {Object} request
+ * @returns {Boolean}
+ */
+function shouldRetry(request) {
+    return lodashGet(request, 'data.shouldRetry', false);
+}
+
+/**
+ * When the request should be retried add it back to the queue
+ * It will either get retried now, or later when we're back online
+ * @param {Object} request
+ */
+function retry(request) {
+    Network.post(request.command, request.data, request.type, request.shouldUseSecure)
+        .then(request.resolve)
+        .catch(request.reject);
+}
+
 /**
  * Adds default values to our request data
  *
@@ -92,35 +112,12 @@ Network.registerParameterEnhancer(addDefaultValuesToParameters);
  * Function used to handle expired auth tokens. It re-authenticates with the API and
  * then replays the original request
  *
- * @param {String} originalCommand
- * @param {Object} [originalParameters]
- * @param {String} [originalType]
- * @returns {Promise}
+ * @param {Object} originalRequest
  */
-function handleExpiredAuthToken(originalCommand, originalParameters, originalType) {
-    // When the authentication process is running, and more API requests will be requeued and they will
-    // be performed after authentication is done.
-    if (isAuthenticating) {
-        return Network.post(originalCommand, originalParameters, originalType);
-    }
-
-    // Prevent any more requests from being processed while authentication happens
-    Network.pauseRequestQueue();
-    isAuthenticating = true;
-
+function handleExpiredAuthToken(originalRequest) {
     // eslint-disable-next-line no-use-before-define
-    return reauthenticate(originalCommand)
-        .then(() => {
-            // Now that the API is authenticated, make the original request again with the new authToken
-            const params = addDefaultValuesToParameters(originalCommand, originalParameters);
-            return Network.post(originalCommand, params, originalType);
-        })
-        .catch(() => (
-
-            // If the request did not succeed because of a networking issue or the server did not respond requeue the
-            // original request.
-            Network.post(originalCommand, originalParameters, originalType)
-        ));
+    reauthenticate(originalRequest.command)
+        .finally(() => retry(originalRequest));
 }
 
 Network.registerRequestHandler((queuedRequest, finalParameters) => {
@@ -159,15 +156,12 @@ Network.registerResponseHandler((queuedRequest, response) => {
         // There are some API requests that should not be retried when there is an auth failure like
         // creating and deleting logins. In those cases, they should handle the original response instead
         // of the new response created by handleExpiredAuthToken.
-        const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry');
-        if (!shouldRetry || unableToReauthenticate) {
+        if (!shouldRetry(queuedRequest) || unableToReauthenticate) {
             queuedRequest.resolve(response);
             return;
         }
 
-        handleExpiredAuthToken(queuedRequest.command, queuedRequest.data, queuedRequest.type)
-            .then(queuedRequest.resolve)
-            .catch(queuedRequest.reject);
+        handleExpiredAuthToken(queuedRequest);
         return;
     }
 
@@ -191,6 +185,13 @@ Network.registerErrorHandler((queuedRequest, error) => {
     // Set an error state and signify we are done loading
     setSessionLoadingAndError(false, 'Cannot connect to server');
 
+    // When the request should be retried add it back to the queue
+    // It will either get retried now, or later when we're back online
+    if (shouldRetry(queuedRequest)) {
+        retry(queuedRequest);
+        return;
+    }
+
     // Reject the queued request with an API offline error so that the original caller can handle it.
     queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE));
 });
@@ -205,7 +206,7 @@ Network.registerErrorHandler((queuedRequest, error) => {
  * @param {String} [parameters.twoFactorAuthCode]
  * @param {String} [parameters.email]
  * @param {String} [parameters.authToken]
- * @returns {Promise}
+ * @returns {Promise<{jsonCode: Number}>}
  */
 function Authenticate(parameters) {
     const commandName = 'Authenticate';
@@ -217,7 +218,15 @@ function Authenticate(parameters) {
         'partnerUserSecret',
     ], parameters, commandName);
 
-    return Network.post(commandName, {
+    // Don't run more than one Authentication requests at the same time
+    if (pendingAuthTask) {
+        return pendingAuthTask;
+    }
+
+    // Make any other requests wait while authentication happens
+    Network.pauseRequestQueue();
+
+    pendingAuthTask = Network.post(commandName, {
         // When authenticating for the first time, we pass useExpensifyLogin as true so we check
         // for credentials for the expensify partnerID to let users Authenticate with their expensify user
         // and password.
@@ -264,7 +273,15 @@ function Authenticate(parameters) {
                 }
             }
             return response;
+        })
+        .finally(() => {
+            // The authentication process is finished so the network can be unpaused to continue
+            // processing requests
+            pendingAuthTask = null;
+            Network.unpauseRequestQueue();
         });
+
+    return pendingAuthTask;
 }
 
 /**
@@ -292,18 +309,9 @@ function reauthenticate(command = '') {
             // new authToken
             updateSessionAuthTokens(response.authToken, response.encryptedAuthToken);
             authToken = response.authToken;
-
-            // The authentication process is finished so the network can be unpaused to continue
-            // processing requests
-            isAuthenticating = false;
-            Network.unpauseRequestQueue();
         })
 
         .catch((error) => {
-            // If authentication fails, then the network can be unpaused
-            Network.unpauseRequestQueue();
-            isAuthenticating = false;
-
             // When a fetch() fails and the "API is offline" error is thrown we won't log the user out. Most likely they
             // have a spotty connection and will need to try to reauthenticate when they come back online. We will
             // re-throw this error so it can be handled by callers of reauthenticate().

src/libs/Network.js

@@ -261,9 +261,9 @@ function canProcessRequestImmediately(request) {
  *
  * @param {String} command
  * @param {*} [data]
- * @param {String} [type]
- * @param {Boolean} shouldUseSecure - Whether we should use the secure API
- * @returns {Promise}
+ * @param {String} [type='post']
+ * @param {Boolean} [shouldUseSecure=false] - Whether we should use the secure API
+ * @returns {Promise<{ jsonCode: Number }>}
  */
 function post(command, data = {}, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) {
     return new Promise((resolve, reject) => {

src/libs/actions/Session/index.js

@@ -424,20 +424,29 @@ function validateEmail(accountID, validateCode, password) {
         });
 }
 
-// It's necessary to throttle requests to reauthenticate since calling this multiple times will cause Pusher to
-// reconnect each time when we only need to reconnect once. This way, if an authToken is expired and we try to
+// We only need to reconnect once. If an authToken is expired, and we try to
 // subscribe to a bunch of channels at once we will only reauthenticate and force reconnect Pusher once.
-const reauthenticatePusher = _.throttle(() => {
+let isReconnectingPusher = false;
+function reauthenticatePusher() {
+    if (isReconnectingPusher) {
+        return;
+    }
+
+    isReconnectingPusher = true;
     Log.info('[Pusher] Re-authenticating and then reconnecting');
+
     API.reauthenticate('Push_Authenticate')
         .then(Pusher.reconnect)
         .catch(() => {
             console.debug(
                 '[PusherConnectionManager]',
                 'Unable to re-authenticate Pusher because we are offline.',
             );
+        })
+        .finally(() => {
+            isReconnectingPusher = false;
         });
-}, 5000, {trailing: false});
+}
 
 /**
  * @param {String} socketID

tests/unit/APITest.js

@@ -0,0 +1,50 @@
+import _ from 'underscore';
+import * as API from '../../src/libs/API';
+import * as Network from '../../src/libs/Network';
+import HttpUtils from '../../src/libs/HttpUtils';
+import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
+import CONFIG from '../../src/CONFIG';
+
+Network.setIsReady(true);
+
+test('Authenticate should not make parallel auth requests', () => {
+    // We're setting up a basic case where all requests succeed
+    const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValue({jsonCode: 200});
+
+    // Given multiple auth calls happening at the same time
+    _.each(_.range(5), () => API.Authenticate({
+        partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
+        partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
+        partnerUserID: 'testUserId',
+        partnerUserSecret: 'testUserSecret',
+    }));
+
+    // Then a single auth request should be made
+    return waitForPromisesToResolve()
+        .then(() => {
+            expect(xhr).toHaveBeenCalledTimes(1);
+        });
+});
+
+test('Multiple Authenticate calls should be resolved with the same value', () => {
+    // A mock where only the first xhr responds with 200 and all the rest 999
+    const mockResponse = {jsonCode: 200};
+    jest.spyOn(HttpUtils, 'xhr')
+        .mockResolvedValueOnce(mockResponse)
+        .mockResolvedValue({jsonCode: 999});
+
+    // Given multiple auth calls happening at the same time
+    const tasks = _.map(_.range(5), () => API.Authenticate({
+        partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
+        partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
+        partnerUserID: 'testUserId',
+        partnerUserSecret: 'testUserSecret',
+    }));
+
+    // Then they all should be resolved from the first response
+    return Promise.all(tasks)
+        .then((results) => {
+            expect(_.size(results)).toEqual(5);
+            _.each(results, response => expect(response).toBe(mockResponse));
+        });
+});

tests/unit/NetworkTest.js

@@ -132,22 +132,8 @@ test('consecutive API calls eventually succeed when authToken is expired', () =>
     const TEST_CHAT_LIST = [1, 2, 3];
 
     let chatList;
-    Onyx.connect({
-        key: 'test_chatList',
-        callback: val => chatList = val,
-    });
-
     let account;
-    Onyx.connect({
-        key: 'test_account',
-        callback: val => account = val,
-    });
-
     let personalDetailsList;
-    Onyx.connect({
-        key: 'test_personalDetailsList',
-        callback: val => personalDetailsList = val,
-    });
 
     // When we sign in
     return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN)
@@ -175,36 +161,34 @@ test('consecutive API calls eventually succeed when authToken is expired', () =>
                     authToken: 'qwerty12345',
                 }))
 
-                // Get&returnValueList=personalDetailsList
-                .mockImplementationOnce(() => Promise.resolve({
-                    jsonCode: 200,
-                    personalDetailsList: TEST_PERSONAL_DETAILS,
-                }))
+                .mockImplementation((command, data) => {
+                    const response = {jsonCode: 200};
 
-                // Get&returnValueList=account
-                .mockImplementationOnce(() => Promise.resolve({
-                    jsonCode: 200,
-                    account: TEST_ACCOUNT_DATA,
-                }))
+                    // Get&returnValueList=chatList
+                    // Get&returnValueList=personalDetailsList
+                    // Get&returnValueList=account
+                    switch (data.returnValueList) {
+                        case 'chatList': response.chatList = TEST_CHAT_LIST; break;
+                        case 'personalDetailsList': response.personalDetailsList = TEST_PERSONAL_DETAILS; break;
+                        case 'account': response.account = TEST_ACCOUNT_DATA; break;
+                        default: throw new Error('Unexpected XHR call');
+                    }
 
-                // Get&returnValueList=chatList
-                .mockImplementationOnce(() => Promise.resolve({
-                    jsonCode: 200,
-                    chatList: TEST_CHAT_LIST,
-                }));
+                    return Promise.resolve(response);
+                });
 
             // And then make 3 API requests in quick succession with an expired authToken and handle the response
             API.Get({returnValueList: 'chatList'})
                 .then((response) => {
-                    Onyx.merge('test_chatList', response.chatList);
+                    chatList = response.chatList;
                 });
             API.Get({returnValueList: 'personalDetailsList'})
                 .then((response) => {
-                    Onyx.merge('test_personalDetailsList', response.personalDetailsList);
+                    personalDetailsList = response.personalDetailsList;
                 });
             API.Get({returnValueList: 'account'})
                 .then((response) => {
-                    Onyx.merge('test_account', response.account);
+                    account = response.account;
                 });
 
             return waitForPromisesToResolve();
@@ -271,3 +255,35 @@ test('retry network request if auth and credentials are not read from Onyx yet',
         expect(spyHttpUtilsXhr).toHaveBeenCalled();
     });
 });
+
+test('retry network request if connection is lost while request is running', () => {
+    // Given a xhr mock that will fail as if network connection dropped
+    const xhr = jest.spyOn(HttpUtils, 'xhr')
+        .mockImplementationOnce(() => {
+            Onyx.merge(ONYXKEYS.NETWORK, {isOffline: true});
+            return Promise.reject(new Error('Network request failed'));
+        })
+        .mockResolvedValue({jsonCode: 200, fromRetriedResult: true});
+
+    // Given a regular "retriable" request (that is bound to fail)
+    const promise = Network.post('Get');
+
+    return waitForPromisesToResolve()
+        .then(() => {
+            // When network connection is recovered
+            Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false});
+            return waitForPromisesToResolve();
+        })
+        .then(() => {
+            // Advance the network request queue by 1 second so that it can realize it's back online
+            jest.advanceTimersByTime(1000);
+            return waitForPromisesToResolve();
+        })
+        .then(() => {
+            // Then the request should be attempted again
+            expect(xhr).toHaveBeenCalledTimes(2);
+
+            // And the promise should be resolved with the 2nd call that succeeded
+            return expect(promise).resolves.toEqual({jsonCode: 200, fromRetriedResult: true});
+        });
+});