Skip to content

Commit

Permalink
fetchActions: Add a timeout to tryFetch.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
chrisbobbe authored and gnprice committed Jul 16, 2021
1 parent bf28c95 commit 93ec9d8
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 20 deletions.
6 changes: 6 additions & 0 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const isDevelopment = process.env.NODE_ENV === 'development';

type Config = {|
requestLongTimeoutMs: number,
messagesPerRequest: number,
messageListThreshold: number,
enableReduxLogging: boolean,
Expand All @@ -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,
Expand Down
22 changes: 21 additions & 1 deletion src/message/__tests__/fetchActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -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', () => {
Expand Down
59 changes: 40 additions & 19 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<T>(func: () => Promise<T>): Promise<T> {
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<undefined> could be returned,
// which is inconsistent with the stated Promise<T> return type.
// eslint-disable-next-line no-unreachable
throw new Error();
}

/**
Expand Down Expand Up @@ -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.',
Expand Down

0 comments on commit 93ec9d8

Please sign in to comment.