From 93ec9d89f86e7adf8d5572d7303de0f0f210f380 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 17 Jun 2020 14:47:57 -0700 Subject: [PATCH] fetchActions: Add a timeout to `tryFetch`. So far, `tryFetch`'s only callers are in the initial fetch; so, add handling for the `TimeoutError` there. The choice of value for `requestLongTimeoutMs` comes from a suggestion in #4165's description: > As a minimal version, if the initial fetch takes a completely > unreasonable amount of time (maybe one minute), we should give up > and take you to the account-picker screen so you can try a > different account and server. Fixes: #4165 --- src/config.js | 6 +++ src/message/__tests__/fetchActions-test.js | 22 +++++++- src/message/fetchActions.js | 59 +++++++++++++++------- 3 files changed, 67 insertions(+), 20 deletions(-) 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 f992359947..e428554986 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -21,7 +21,7 @@ import { GravatarURL } from '../../utils/avatar'; import * as eg from '../../__tests__/lib/exampleData'; import { ApiError, Server5xxError, NetworkError } from '../../api/apiErrors'; import { fakeSleep } from '../../__tests__/lib/fakeTimers'; -import { BackoffMachine } from '../../utils/async'; +import { TimeoutError, BackoffMachine } from '../../utils/async'; import * as logging from '../../utils/logging'; const mockStore = configureStore([thunk]); @@ -201,6 +201,26 @@ describe('fetchActions', () => { jest.runAllTimers(); }); + + test('times out after many short-duration 5xx errors', async () => { + const func = jest.fn(async () => { + await fakeSleep(50); + throw new Server5xxError(500); + }); + + 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 34faeb191f..7a7a988e5d 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -31,7 +31,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'; @@ -182,8 +182,6 @@ const initialFetchAbortPlain = (reason: InitialFetchAbortReason): Action => ({ reason, }); -// This will be used in an upcoming commit. -/* eslint-disable-next-line no-unused-vars */ export const initialFetchAbort = (reason: InitialFetchAbortReason) => async ( dispatch: Dispatch, getState: GetState, @@ -303,32 +301,53 @@ const fetchPrivateMessages = () => async (dispatch: Dispatch, getState: GetState /** * Makes a request, retrying on server/network operational errors until - * success. + * success, with timeout. * * Waits between retries with a backoff. * * Other, non-retryable errors (client errors and all unexpected errors) * 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. The time spent waiting 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 (!(e instanceof Server5xxError || e instanceof NetworkError)) { - 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 (!(e instanceof Server5xxError || e instanceof NetworkError)) { + 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 +410,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.',