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 Jul 16, 2020
1 parent 9cc1f61 commit 180bbd9
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 24 deletions.
42 changes: 40 additions & 2 deletions src/message/__tests__/fetchActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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');
Expand All @@ -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', () => {
Expand Down
69 changes: 47 additions & 22 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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<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 @@ -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;
}

Expand Down

0 comments on commit 180bbd9

Please sign in to comment.