Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stop "retrying" requests that fail in the main queue #8860

Merged
merged 8 commits into from
May 26, 2022
Merged

Stop "retrying" requests that fail in the main queue #8860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
167764a
remove shouldRetry and general retryability of non persistable reques…
marcaaron May 3, 2022
d12816d
fix conflicts
marcaaron May 4, 2022
28fb3a1
Fix tests
marcaaron May 4, 2022
3e56fd9
remove caca
marcaaron May 4, 2022
e6808e5
Fix test
marcaaron May 4, 2022
64df018
Do not catch personal details error
marcaaron May 4, 2022
5c72df0
Fix tests
marcaaron May 17, 2022
0aaf3e2
Revert import and param update
marcaaron May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 0 additions & 38 deletions src/libs/Middleware/Retry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,7 @@
import lodashGet from 'lodash/get';
import RetryCounter from '../RetryCounter';
import * as PersistedRequests from '../actions/PersistedRequests';
import * as MainQueue from '../Network/MainQueue';
import Log from '../Log';
import CONST from '../../CONST';

// Keep track of retries for any non-persisted requests
const mainQueueRetryCounter = new RetryCounter();

/**
* @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);
Log.info('[Network] A retryable request failed', false, {
retryCount,
command: queuedRequest.command,
error: error.message,
});

if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) {
MainQueue.push(queuedRequest);
return true;
}

Log.info('[Network] Request was retried too many times with no success. No more retries left');
return false;
}

/**
* @param {Promise} response
* @param {Object} request
Expand All @@ -55,10 +21,6 @@ function Retry(response, request, isFromSequentialQueue) {
return;
}

if (retryFailedRequest(request, error)) {
return;
}

if (request.command !== 'Log') {
Log.hmmm('[Network] Handled error when making request', error);
} else {
Expand Down
4 changes: 2 additions & 2 deletions src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,9 @@ function setPersonalDetails(details, shouldGrowl) {
Growl.error(Localize.translateLocal('personalDetails.error.firstNameLength'), 3000);
} else if (response.jsonCode === 401) {
Growl.error(Localize.translateLocal('personalDetails.error.lastNameLength'), 3000);
} else {
console.debug('Error while setting personal details', response);
}
}).catch((error) => {
console.debug('Error while setting personal details', error);
});
}

Expand Down
59 changes: 32 additions & 27 deletions tests/unit/NetworkTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,16 +274,15 @@ test('Request will not run until credentials are read from Onyx', () => {
});
});

test('retry network request if connection is lost while request is running', () => {
test('Non-retryable request will not be retried if connection is lost in flight', () => {
// 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(CONST.ERROR.FAILED_TO_FETCH));
})
.mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS, fromRetriedResult: true});
});

// Given a regular "retryable" request (that is bound to fail)
// Given a non-retryable request (that is bound to fail)
const promise = Network.post('Get');

return waitForPromisesToResolve()
Expand All @@ -298,15 +297,15 @@ test('retry network request if connection is lost while request is running', ()
return waitForPromisesToResolve();
})
.then(() => {
// Then the request should be attempted again
expect(xhr).toHaveBeenCalledTimes(2);
// Then the request should only have been attempted once and we should get an unable to retry
expect(xhr).toHaveBeenCalledTimes(1);

// And the promise should be resolved with the 2nd call that succeeded
return expect(promise).resolves.toEqual({jsonCode: CONST.JSON_CODE.SUCCESS, fromRetriedResult: true});
// And the promise should be resolved with the special offline jsonCode
return expect(promise).resolves.toEqual({jsonCode: CONST.JSON_CODE.UNABLE_TO_RETRY});
});
});

test('requests should be persisted while offline', () => {
test('Retryable requests should be persisted while offline', () => {
// We don't expect calls `xhr` so we make the test fail if such call is made
const xhr = jest.spyOn(HttpUtils, 'xhr').mockRejectedValue(new Error('Unexpected xhr call'));

Expand All @@ -328,15 +327,25 @@ test('requests should be persisted while offline', () => {
expect.objectContaining({command: 'mock command', data: expect.objectContaining({param1: 'value1'})}),
expect.objectContaining({command: 'mock command', data: expect.objectContaining({param3: 'value3'})}),
]);

PersistedRequests.clear();
return waitForPromisesToResolve();
})
.then(() => {
expect(PersistedRequests.getAll()).toEqual([]);
});
});

test('requests should resume when we are online', () => {
test('Retryable requests should resume when we are online', () => {
// We're setting up a basic case where all requests succeed when we resume connectivity
const xhr = jest.spyOn(HttpUtils, 'xhr').mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS});

// Given we have some requests made while we're offline
return Onyx.set(ONYXKEYS.NETWORK, {isOffline: true})
return Onyx.multiSet({
[ONYXKEYS.NETWORK]: {isOffline: true},
[ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'},
[ONYXKEYS.SESSION]: {authToken: 'testToken'},
})
.then(() => {
// When network calls with `persist` are made
Network.post('mock command', {param1: 'value1', persist: true});
Expand All @@ -352,6 +361,9 @@ test('requests should resume when we are online', () => {
.then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: false}))
.then(waitForPromisesToResolve)
.then(() => {
expect(NetworkStore.isOffline()).toBe(false);
expect(SequentialQueue.isRunning()).toBe(false);

// Then `xhr` should be called with expected data, and the persisted queue should be empty
expect(xhr).toHaveBeenCalledTimes(2);
expect(xhr.mock.calls).toEqual([
Expand Down Expand Up @@ -466,19 +478,19 @@ test('test bad response will log alert', () => {
});
});

test('test Failed to fetch error for requests not flagged with shouldRetry will resolve with an offline jsonCode', () => {
// Setup xhr handler that rejects once with a 502 Bad Gateway
test('test Failed to fetch error for non-retryable requests resolve with unable to retry jsonCode', () => {
// Setup xhr handler that rejects once with a Failed to Fetch
global.fetch = jest.fn().mockRejectedValue(new Error(CONST.ERROR.FAILED_TO_FETCH));

const onResolved = jest.fn();

// Given we have a request made while online
return Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})
.then(() => {
expect(NetworkStore.isOffline()).toBe(false);

// When network calls with are made
Network.post('MockCommand', {param1: 'value1', shouldRetry: false})
Network.post('mock command', {param1: 'value1'})
.then(onResolved);

return waitForPromisesToResolve();
})
.then(() => {
Expand All @@ -497,7 +509,7 @@ test('persisted request can trigger reauthentication for anything retryable', ()
.mockResolvedValueOnce({jsonCode: CONST.JSON_CODE.SUCCESS}); // Original command return 200

// Given we have a request made while we're offline and we have credentials available to reauthenticate
Onyx.merge(ONYXKEYS.CREDENTIALS, {autoGeneratedLogin: 'caca', autoGeneratedPassword: 'caca'});
Onyx.merge(ONYXKEYS.CREDENTIALS, {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'});
return waitForPromisesToResolve()
.then(() => Onyx.set(ONYXKEYS.NETWORK, {isOffline: true}))
.then(() => {
Expand Down Expand Up @@ -527,14 +539,13 @@ test('several actions made while offline will get added in the order they are cr
// Given offline state where all requests will eventualy succeed without issue
const xhr = jest.spyOn(HttpUtils, 'xhr')
.mockResolvedValue({jsonCode: CONST.JSON_CODE.SUCCESS});

return Onyx.multiSet({
[ONYXKEYS.SESSION]: {authToken: 'anyToken'},
[ONYXKEYS.NETWORK]: {isOffline: true},
[ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test_user', autoGeneratedPassword: 'psswd'},
})
.then(() => {
// When we queue 6 persistable commands
// When we queue 6 persistable commands and one not persistable
Network.post('MockCommand', {content: 'value1', persist: true});
Network.post('MockCommand', {content: 'value2', persist: true});
Network.post('MockCommand', {content: 'value3', persist: true});
Expand All @@ -557,13 +568,6 @@ test('several actions made while offline will get added in the order they are cr
expect(xhr.mock.calls[3][1].content).toBe('value4');
expect(xhr.mock.calls[4][1].content).toBe('value5');
expect(xhr.mock.calls[5][1].content).toBe('value6');

// Move main queue forward so it processes the "read" request
jest.advanceTimersByTime(CONST.NETWORK.PROCESS_REQUEST_DELAY_MS);
return waitForPromisesToResolve();
})
.then(() => {
expect(xhr.mock.calls[6][1].content).toBe('not-persisted');
});
});

Expand All @@ -575,7 +579,8 @@ test('several actions made while offline will get added in the order they are cr

return Onyx.multiSet({
[ONYXKEYS.NETWORK]: {isOffline: true},
[ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'caca', autoGeneratedPassword: 'caca'},
[ONYXKEYS.SESSION]: {authToken: 'test'},
[ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin: 'test', autoGeneratedPassword: 'passwd'},
})
.then(() => {
// When we queue 6 persistable commands
Expand Down