Skip to content

Commit

Permalink
initial fetch: Add a timeout to abort.
Browse files Browse the repository at this point in the history
Using `INITIAL_FETCH_ABORT` and `promiseTimeout`, which we set up in
the last few commits, abort and go to the accounts screen if the
initial fetch is taking too long.

Fixes: zulip#4165
  • Loading branch information
chrisbobbe committed Apr 13, 2021
1 parent 78999a5 commit a21e619
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 23 deletions.
26 changes: 25 additions & 1 deletion src/message/__tests__/fetchActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { GravatarURL } from '../../utils/avatar';
import * as eg from '../../__tests__/lib/exampleData';
import { ApiError } from '../../api/apiErrors';
import { fakeSleep } from '../../__tests__/lib/fakeTimers';
import { BackoffMachine } from '../../utils/async';
import { TimeoutError, BackoffMachine } from '../../utils/async';

const mockStore = configureStore([thunk]);

Expand Down Expand Up @@ -138,6 +138,30 @@ describe('fetchActions', () => {

jest.runAllTimers();
});

test('times out after many short-duration 5xx errors', async () => {
const func = jest.fn(async () => {
await fakeSleep(50);
throw new ApiError(500, {
code: 'SOME_ERROR_CODE',
msg: 'Internal Server Error',
result: 'error',
});
});

await expect(tryFetch(func)).rejects.toThrow(TimeoutError);

expect(func.mock.calls.length).toBeGreaterThan(50);
});

test('times out after hanging on one request', async () => {
const tryFetchPromise = tryFetch(async () => {
await new Promise((resolve, reject) => {});
});

await fakeSleep(60000);
return expect(tryFetchPromise).rejects.toThrow(TimeoutError);
});
});

describe('fetchMessages', () => {
Expand Down
69 changes: 47 additions & 22 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import {
} from '../actionConstants';
import { FIRST_UNREAD_ANCHOR, LAST_MESSAGE_ANCHOR } from '../anchor';
import { ALL_PRIVATE_NARROW, apiNarrowOfNarrow } from '../utils/narrow';
import { BackoffMachine } from '../utils/async';
import { BackoffMachine, promiseTimeout, TimeoutError } from '../utils/async';
import * as logging from '../utils/logging';
import { initNotifications } from '../notification/notificationActions';
import { addToOutbox, sendOutbox } from '../outbox/outboxActions';
import { realmInit } from '../realm/realmActions';
Expand Down Expand Up @@ -175,8 +176,6 @@ const initialFetchAbortPlain = (): Action => ({
type: INITIAL_FETCH_ABORT,
});

// This will be used in an upcoming commit.
/* eslint-disable-next-line no-unused-vars */
export const initialFetchAbort = () => async (dispatch: Dispatch, getState: GetState) => {
NavigationService.dispatch(resetToAccountPicker());
dispatch(initialFetchAbortPlain());
Expand Down Expand Up @@ -271,26 +270,46 @@ const fetchPrivateMessages = () => async (dispatch: Dispatch, getState: GetState
* If the function is an API call and the response has HTTP status code 4xx
* the error is considered unrecoverable and the exception is rethrown, to be
* handled further up in the call stack.
*
* After a certain duration, times out with a TimeoutError.
*/
export async function tryFetch<T>(func: () => Promise<T>): Promise<T> {
const MAX_TIME_MS: number = 60000;
const backoffMachine = new BackoffMachine();
// eslint-disable-next-line no-constant-condition
while (true) {
try {
return await func();
} catch (e) {
if (isClientError(e)) {
throw e;
}
await backoffMachine.wait();
}
}

// Without this, Flow 0.92.1 does not know this code is unreachable,
// and it incorrectly thinks Promise<undefined> could be returned,
// which is inconsistent with the stated Promise<T> return type.
// eslint-disable-next-line no-unreachable
throw new Error();
// TODO: Use AbortController instead of this stateful flag; #4170
let timerHasExpired = false;

return promiseTimeout(
(async () => {
// eslint-disable-next-line no-constant-condition
while (true) {
if (timerHasExpired) {
// No one is listening for this Promise's outcome, so stop
// doing more work.
throw new Error();
}
try {
return await func();
} catch (e) {
if (isClientError(e)) {
throw e;
}
await backoffMachine.wait();
}
}
// Without this, Flow 0.92.1 does not know this code is unreachable,
// and it incorrectly thinks Promise<undefined> could be returned,
// which is inconsistent with the stated Promise<T> return type.
// eslint-disable-next-line no-unreachable
throw new Error();
})(),
MAX_TIME_MS,
() => {
timerHasExpired = true;
throw new TimeoutError();
},
);
}

/**
Expand Down Expand Up @@ -339,9 +358,15 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat
tryFetch(() => api.getServerSettings(auth.realm)),
]);
} catch (e) {
// This should only happen on a 4xx HTTP status, which should only
// happen when `auth` is no longer valid. No use retrying; just log out.
dispatch(logout());
if (isClientError(e)) {
dispatch(logout());
} else if (e instanceof TimeoutError) {
dispatch(initialFetchAbort());
} else {
logging.warn(e, {
message: 'Unexpected error during initial fetch and serverSettings fetch.',
});
}
return;
}

Expand Down

0 comments on commit a21e619

Please sign in to comment.