From 180bbd973f5ade9ab6df73f5644a65fd57780933 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 17 Jun 2020 14:47:57 -0700 Subject: [PATCH] initial fetch: Add a timeout to abort. 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: #4165 --- src/message/__tests__/fetchActions-test.js | 42 ++++++++++++- src/message/fetchActions.js | 69 +++++++++++++++------- 2 files changed, 87 insertions(+), 24 deletions(-) diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index d5d0237cf0a..aad529a3bab 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -5,7 +5,7 @@ import { streamNarrow, HOME_NARROW, HOME_NARROW_STR } from '../../utils/narrow'; import { navStateWithNarrow } from '../../utils/testHelpers'; import { ApiError } from '../../api/apiErrors'; import { fakeSleep } from '../../__tests__/lib/fakeTimers'; -import { BackoffMachine } from '../../utils/async'; +import { TimeoutError, BackoffMachine } from '../../utils/async'; const narrow = streamNarrow('some stream'); const streamNarrowStr = JSON.stringify(narrow); @@ -64,6 +64,7 @@ describe('fetchActions', () => { await expect(tryFetchFunc.mock.results[0]?.value).rejects.toThrow('First run exception'); await expect(tryFetchFunc.mock.results[1]?.value).resolves.toBe('hello'); + await fakeSleep(100000); const result = await tryFetchPromise; expect(result).toEqual('hello'); @@ -81,12 +82,49 @@ describe('fetchActions', () => { }); const tryFetchPromise = tryFetch(func); - await jest.runAllTimers(); + // The number of iterations is experimentally determined. I'm + // not sure what's going on here. + for (let i = 0; i < 3; i++) { + await Promise.resolve(); + } expect(func).toHaveBeenCalledTimes(1); + await fakeSleep(60000); + return expect(tryFetchPromise).rejects.toThrow(apiError); }); + + 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', + }); + }); + const tryFetchPromise = tryFetch(func); + + // The number of iterations is experimentally determined. I'm + // not sure what's going on here. + for (let i = 0; i < 500; i++) { + await Promise.resolve(); + } + + expect(func.mock.calls.length).toBeGreaterThan(50); + + return expect(tryFetchPromise).rejects.toThrow(TimeoutError); + }); + + 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 be3c4654c8a..322cd210cc8 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -30,7 +30,8 @@ import { } from '../actionConstants'; import { FIRST_UNREAD_ANCHOR, LAST_MESSAGE_ANCHOR } from '../anchor'; import { ALL_PRIVATE_NARROW } 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'; @@ -128,8 +129,6 @@ const initialFetchComplete = (): Action => ({ type: INITIAL_FETCH_COMPLETE, }); -// This will be used in an upcoming commit. -/* eslint-disable-next-line no-unused-vars */ const initialFetchAbort = (): Action => ({ type: INITIAL_FETCH_ABORT, }); @@ -234,26 +233,46 @@ const fetchTopMostNarrow = () => 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(func: () => Promise): Promise { + 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 could be returned, - // which is inconsistent with the stated Promise 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 could be returned, + // which is inconsistent with the stated Promise return type. + // eslint-disable-next-line no-unreachable + throw new Error(); + })(), + MAX_TIME_MS, + () => { + timerHasExpired = true; + throw new TimeoutError(); + }, + ); } /** @@ -294,9 +313,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; }