diff --git a/src/config.js b/src/config.js index e0b6d28ad9..d4c34613e0 100644 --- a/src/config.js +++ b/src/config.js @@ -2,6 +2,7 @@ const isDevelopment = process.env.NODE_ENV === 'development'; type Config = {| + requestLongTimeoutMs: number, messagesPerRequest: number, messageListThreshold: number, enableReduxLogging: boolean, @@ -14,6 +15,11 @@ type Config = {| |}; const config: Config = { + // A completely unreasonable amount of time for a request, or + // several retries of a request, to take. If this elapses, we're + // better off giving up. + requestLongTimeoutMs: 60 * 1000, + messagesPerRequest: 100, messageListThreshold: 4000, enableReduxLogging: isDevelopment && !!global.btoa, diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index d3b9d46714..d4571d6533 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -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]); @@ -176,6 +176,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', () => { diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index b4d4f76bc5..e03142f63c 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -30,7 +30,7 @@ import { import { FIRST_UNREAD_ANCHOR, LAST_MESSAGE_ANCHOR } from '../anchor'; import { showErrorAlert } from '../utils/info'; import { ALL_PRIVATE_NARROW, apiNarrowOfNarrow } from '../utils/narrow'; -import { BackoffMachine } from '../utils/async'; +import { BackoffMachine, promiseTimeout, TimeoutError } from '../utils/async'; import { initNotifications } from '../notification/notificationActions'; import { addToOutbox, sendOutbox } from '../outbox/outboxActions'; import { realmInit } from '../realm/realmActions'; @@ -181,8 +181,6 @@ const initialFetchAbortPlain = (reason: 'server' | 'network' | 'timeout'): Actio reason, }); -// This will be used in an upcoming commit. -/* eslint-disable-next-line no-unused-vars */ export const initialFetchAbort = (reason: 'server' | 'network' | 'timeout') => async ( dispatch: Dispatch, getState: GetState, @@ -304,31 +302,53 @@ const fetchPrivateMessages = () => async (dispatch: Dispatch, getState: GetState }; /** - * Makes a request that retries until success or a non-retryable error. + * Makes a request that retries until success or a non-retryable error, with + * timeout. * * Waits between retries with a backoff. * * A non-retryable error will propagate to the caller to be handled. + * + * The timeout's length is `config.requestLongTimeoutMs` and it is absolute: + * it triggers after that time has elapsed no matter whether the time was + * spent waiting to hear back from one request, or retrying a request + * unsuccessfully many times (and the time spent in backoff is included in + * that). */ export async function tryFetch(func: () => Promise): Promise { const backoffMachine = new BackoffMachine(); - // eslint-disable-next-line no-constant-condition - while (true) { - try { - return await func(); - } catch (e) { - if (!isRetryable(e)) { - throw e; - } - await backoffMachine.wait(); + + // TODO: Use AbortController instead of this stateful flag; #4170 + let timerHasExpired = false; + + try { + return await promiseTimeout( + (async () => { + // eslint-disable-next-line no-constant-condition + while (true) { + if (timerHasExpired) { + // No one is listening for this Promise to settle, so stop + // doing more work. + throw new Error(); + } + try { + return await func(); + } catch (e) { + if (!isRetryable(e)) { + throw e; + } + await backoffMachine.wait(); + } + } + })(), + config.requestLongTimeoutMs, + ); + } catch (e) { + if (e instanceof TimeoutError) { + timerHasExpired = true; } + throw e; } - - // Without this, Flow 0.92.1 does not know this code is unreachable, - // and it incorrectly thinks Promise could be returned, - // which is inconsistent with the stated Promise return type. - // eslint-disable-next-line no-unreachable - throw new Error(); } /** @@ -391,6 +411,8 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat // This should only happen when `auth` is no longer valid. No // use retrying; just log out. dispatch(logout()); + } else if (e instanceof TimeoutError) { + dispatch(initialFetchAbort('timeout')); } else { logging.warn(e, { message: 'Unexpected error during initial fetch and serverSettings fetch.',