Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time out initial fetch and go to account-picker screen. #4754

Merged
merged 13 commits into from
Jul 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/actionConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export const REALM_INIT: 'REALM_INIT' = 'REALM_INIT';

export const INITIAL_FETCH_START: 'INITIAL_FETCH_START' = 'INITIAL_FETCH_START';
export const INITIAL_FETCH_COMPLETE: 'INITIAL_FETCH_COMPLETE' = 'INITIAL_FETCH_COMPLETE';
export const INITIAL_FETCH_ABORT: 'INITIAL_FETCH_ABORT' = 'INITIAL_FETCH_ABORT';
export const INIT_TOPICS: 'INIT_TOPICS' = 'INIT_TOPICS';

export const EVENT: 'EVENT' = 'EVENT';
Expand Down
22 changes: 20 additions & 2 deletions src/actionTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
MESSAGE_FETCH_COMPLETE,
INITIAL_FETCH_START,
INITIAL_FETCH_COMPLETE,
INITIAL_FETCH_ABORT,
SETTINGS_CHANGE,
DRAFT_UPDATE,
PRESENCE_RESPONSE,
Expand Down Expand Up @@ -194,7 +195,7 @@ type MessageFetchStartAction = {|
/**
* Any unexpected failure in a message fetch.
*
* Includes internal server errors and any errors we throw when
* Includes request timeout errors and any errors we throw when
* validating and reshaping the server data at the edge.
*
* In an ideal crunchy-shell world [1], none of these will be thrown
Expand Down Expand Up @@ -235,6 +236,19 @@ type InitialFetchCompleteAction = {|
type: typeof INITIAL_FETCH_COMPLETE,
|};

export type InitialFetchAbortReason = 'server' | 'network' | 'timeout' | 'unexpected';

/**
* Notify Redux that we've given up on the initial fetch.
*
* Not for unrecoverable errors, like ApiErrors, which indicate that we
* tried and failed, not that we gave up trying.
*/
type InitialFetchAbortAction = {|
type: typeof INITIAL_FETCH_ABORT,
reason: InitialFetchAbortReason,
|};

type ServerEvent = {|
id: number,
|};
Expand Down Expand Up @@ -602,7 +616,11 @@ type InitTopicsAction = {|

type AccountAction = AccountSwitchAction | AccountRemoveAction | LoginSuccessAction | LogoutAction;

type LoadingAction = DeadQueueAction | InitialFetchStartAction | InitialFetchCompleteAction;
type LoadingAction =
| DeadQueueAction
| InitialFetchStartAction
| InitialFetchCompleteAction
| InitialFetchAbortAction;

type MessageAction = MessageFetchStartAction | MessageFetchErrorAction | MessageFetchCompleteAction;

Expand Down
9 changes: 8 additions & 1 deletion src/chat/FetchError.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { StyleSheet, View } from 'react-native';

import type { Narrow } from '../types';
import { Label } from '../common';
import { TimeoutError } from '../utils/async';

const styles = StyleSheet.create({
container: {
Expand All @@ -28,7 +29,13 @@ export default class FetchError extends PureComponent<Props> {
render() {
return (
<View style={styles.container}>
<Label style={styles.text} text="Oops! Something went wrong." />
{(() => {
if (this.props.error instanceof TimeoutError) {
return <Label style={styles.text} text="Request timed out." />;
} else {
return <Label style={styles.text} text="Oops! Something went wrong." />;
}
})()}
Comment on lines +32 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably simpler as a … ? … : … expression, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably simpler as a … ? … : … expression, no?

Sure. I used if / else because I thought there might be other cases to consider in the future, and this form would make it easier to expand on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yeah, that seems like a fine reason.

</View>
);
}
Expand Down
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
162 changes: 116 additions & 46 deletions src/message/__tests__/fetchActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import type { ServerMessage } from '../../api/messages/getMessages';
import { streamNarrow, HOME_NARROW, HOME_NARROW_STR, keyFromNarrow } from '../../utils/narrow';
import { GravatarURL } from '../../utils/avatar';
import * as eg from '../../__tests__/lib/exampleData';
import { ApiError, Server5xxError } from '../../api/apiErrors';
import { 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 @@ -104,65 +104,129 @@ describe('fetchActions', () => {
jest.clearAllTimers();
});

test('resolves any promise, if there is no exception', async () => {
const tryFetchFunc = jest.fn(async () => {
await fakeSleep(10);
return 'hello';
});
describe.each([false, true])(
'whether or not asked to retry; was asked this time? %s',
withRetry => {
test('resolves any promise, if there is no exception', async () => {
const tryFetchFunc = jest.fn(async () => {
await fakeSleep(10);
return 'hello';
});

await expect(tryFetch(tryFetchFunc)).resolves.toBe('hello');
await expect(tryFetch(tryFetchFunc, withRetry)).resolves.toBe('hello');

expect(tryFetchFunc).toHaveBeenCalledTimes(1);
await expect(tryFetchFunc.mock.results[0].value).resolves.toBe('hello');
expect(tryFetchFunc).toHaveBeenCalledTimes(1);
await expect(tryFetchFunc.mock.results[0].value).resolves.toBe('hello');

jest.runAllTimers();
});
jest.runAllTimers();
});

// TODO: test more errors, like regular `new Error()`s. Unexpected
// errors should actually cause the retry loop to break; we'll fix
// that soon.
test('retries a call if there is a non-client error', async () => {
const serverError = new Server5xxError(500);

// fail on first call, succeed second time
let callCount = 0;
const thrower = jest.fn(() => {
callCount++;
if (callCount === 1) {
throw serverError;
}
return 'hello';
});
test('Rethrows an unexpected error without retrying', async () => {
const unexpectedError = new Error('You have displaced the mirth.');

const func = jest.fn(async () => {
throw unexpectedError;
});

await expect(tryFetch(func, withRetry)).rejects.toThrow(unexpectedError);
expect(func).toHaveBeenCalledTimes(1);

jest.runAllTimers();
});

test('times out after hanging on one request', async () => {
const tryFetchPromise = tryFetch(async () => {
await new Promise((resolve, reject) => {});
}, withRetry);

await fakeSleep(60000);
return expect(tryFetchPromise).rejects.toThrow(TimeoutError);
});
},
);

describe('if asked to retry', () => {
test('retries a call if there is a recoverable error', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchActions: Fail early on all non–known-retryable errors in `tryFetch`.

I appreciate your careful use of both an en-dash and a hyphen. Wouldn't want a reader to think we're talking about errors that are retryable in a non-known fashion :-P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I still had access to the Chicago Manual of Style; it looks like this would be covered in Chapter 6: https://www.chicagomanualofstyle.org/16/ch06/ch06_toc.html 😛 (but I no longer have my college-sponsored subscription, alas).

const serverError = new Server5xxError(500);

// fail on first call, succeed second time
let callCount = 0;
const thrower = jest.fn(() => {
callCount++;
if (callCount === 1) {
throw serverError;
}
return 'hello';
});

const tryFetchFunc = jest.fn(async () => {
await fakeSleep(10);
return thrower();
});

await expect(tryFetch(tryFetchFunc, true)).resolves.toBe('hello');

expect(tryFetchFunc).toHaveBeenCalledTimes(2);
await expect(tryFetchFunc.mock.results[0].value).rejects.toThrow(serverError);
await expect(tryFetchFunc.mock.results[1].value).resolves.toBe('hello');

const tryFetchFunc = jest.fn(async () => {
await fakeSleep(10);
return thrower();
jest.runAllTimers();
});

await expect(tryFetch(tryFetchFunc)).resolves.toBe('hello');
test('retries a call if there is a network error', async () => {
const networkError = new NetworkError();

// fail on first call, succeed second time
let callCount = 0;
const thrower = jest.fn(() => {
callCount++;
if (callCount === 1) {
throw networkError;
}
return 'hello';
});

expect(tryFetchFunc).toHaveBeenCalledTimes(2);
await expect(tryFetchFunc.mock.results[0].value).rejects.toThrow(serverError);
await expect(tryFetchFunc.mock.results[1].value).resolves.toBe('hello');
const tryFetchFunc = jest.fn(async () => {
await fakeSleep(10);
return thrower();
});

jest.runAllTimers();
});
await expect(tryFetch(tryFetchFunc)).resolves.toBe('hello');

test('Rethrows a 4xx error without retrying', async () => {
const apiError = new ApiError(400, {
code: 'BAD_REQUEST',
msg: 'Bad Request',
result: 'error',
expect(tryFetchFunc).toHaveBeenCalledTimes(2);
await expect(tryFetchFunc.mock.results[0].value).rejects.toThrow(networkError);
await expect(tryFetchFunc.mock.results[1].value).resolves.toBe('hello');

jest.runAllTimers();
});

const func = jest.fn(async () => {
throw apiError;
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, true)).rejects.toThrow(TimeoutError);

expect(func.mock.calls.length).toBeGreaterThan(50);
});
});

await expect(tryFetch(func)).rejects.toThrow(apiError);
expect(func).toHaveBeenCalledTimes(1);
describe('if not asked to retry', () => {
test('does not retry a call if there is a server error', async () => {
const serverError = new Server5xxError(500);

jest.runAllTimers();
const tryFetchFunc = jest.fn(async () => {
await fakeSleep(10);
throw serverError;
});

await expect(tryFetch(tryFetchFunc, false)).rejects.toThrow(serverError);

expect(tryFetchFunc).toHaveBeenCalledTimes(1);

jest.runAllTimers();
});
});
});

Expand Down Expand Up @@ -245,6 +309,12 @@ describe('fetchActions', () => {
});

describe('failure', () => {
beforeAll(() => {
// suppress `logging.warn` output
// $FlowFixMe[prop-missing]: Jest mock
logging.warn.mockReturnValue();
});

test('rejects when user is not logged in, dispatches MESSAGE_FETCH_ERROR', async () => {
const stateWithoutAccount = {
...baseState,
Expand Down