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.

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

Handle cancelled requests correctly #9101

Merged
merged 3 commits into from
May 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion src/libs/Middleware/Logging.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ function Logging(response, request) {
if (persisted) {
PersistedRequests.remove(request);
}
return;

// Re-throw this error so the next handler can manage it
throw error;
}

if (error.message === CONST.ERROR.FAILED_TO_FETCH) {
Expand Down
6 changes: 6 additions & 0 deletions src/libs/Middleware/Reauthentication.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ import Log from '../Log';
function Reauthentication(response, request, isFromSequentialQueue) {
return response
.then((data) => {
// If there is no data for some reason then we cannot reauthenticate
if (!data) {
Log.hmmm('Undefined data in Reauthentication');
return;
}

if (NetworkStore.isOffline()) {
// If we are offline and somehow handling this response we do not want to reauthenticate
throw new Error('Unable to reauthenticate because we are offline');
Expand Down
7 changes: 5 additions & 2 deletions src/libs/Middleware/RecheckConnection.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ function RecheckConnection(response) {
const cancelRequestTimeoutTimer = startRecheckTimeoutTimer();
return response
.catch((error) => {
// Because we ran into an error we assume we might be offline and do a "connection" health test
NetworkConnection.recheckNetworkConnection();
if (error.name !== CONST.ERROR.REQUEST_CANCELLED) {
// Because we ran into an error we assume we might be offline and do a "connection" health test
NetworkConnection.recheckNetworkConnection();
}

throw error;
})
.finally(cancelRequestTimeoutTimer);
Expand Down
5 changes: 5 additions & 0 deletions src/libs/Middleware/Retry.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ function retryFailedRequest(queuedRequest, error) {
function Retry(response, request, isFromSequentialQueue) {
return response
.catch((error) => {
// Do not retry any requests that are cancelled
if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
return;
}

if (isFromSequentialQueue) {
const retryCount = PersistedRequests.incrementRetries(request);
Log.info('Persisted request failed', false, {retryCount, command: request.command, error: error.message});
Expand Down
8 changes: 8 additions & 0 deletions src/libs/Network/MainQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,17 @@ function clear() {
HttpUtils.cancelPendingRequests();
}

/**
* @returns {Array}
*/
function getAll() {
return networkRequestQueue;
}

export {
clear,
replay,
push,
process,
getAll,
};
24 changes: 24 additions & 0 deletions tests/unit/NetworkTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -765,3 +765,27 @@ test('persistable request will move directly to the SequentialQueue when we are
expect(thirdRequestCommandName).toBe('MockCommandTwo');
});
});

test('cancelled requests should not be retried', () => {
const xhr = jest.spyOn(HttpUtils, 'xhr');

// GIVEN a mock that will return a "cancelled" request error
global.fetch = jest.fn()
.mockRejectedValue(new DOMException('Aborted', CONST.ERROR.REQUEST_CANCELLED));

return Onyx.set(ONYXKEYS.NETWORK, {isOffline: false})
.then(() => {
// WHEN we make a few requests and then cancel them
Network.post('MockCommandOne');
Network.post('MockCommandTwo');
Network.post('MockCommandThree');

// WHEN we wait for the requests to all cancel
return waitForPromisesToResolve();
})
.then(() => {
// THEN expect our queue to be empty and for no requests to have been retried
expect(MainQueue.getAll().length).toBe(0);
expect(xhr.mock.calls.length).toBe(3);
});
});