Skip to content

Commit

Permalink
makeBackoffMachine: Improve backoff sleep logic by making it local.
Browse files Browse the repository at this point in the history
Fixes: 3840.

Replace progressiveTimeout with makeBackoffMachine, which is called
above a loop, and .wait() can be called inside the loop on the
returned backoffMachine. Give access to numSleepsCompleted and
timeElapsedSoFar so "give up" logic (e.g., on too many network
requests with transient failures) can be handled near .wait() call
sites. Prepare for work on #3829 (a permanent timeout on sending
outbox messages).

Previously, the state logic in progressiveTimeout was global, which
meant a potentially desirable general throttle on all network
requests that used it, but the drawbacks were greater. A particular
call to progressiveTimeout was nondeterministic, because other calls
may have been made recently, affecting the resulting delay duration.
A 60-second threshold was used as a heuristic to distinguish request
retry loops from each other, but this is much more effectively done
with the new interface.
  • Loading branch information
Chris Bobbe committed Jan 25, 2020
1 parent 5e290d0 commit 126adf4
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 41 deletions.
4 changes: 3 additions & 1 deletion src/events/eventActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export const startEventPolling = (queueId: number, eventId: number) => async (
) => {
let lastEventId = eventId;

const backoffMachine = makeBackoffMachine();

/* eslint-disable no-await-in-loop */
// eslint-disable-next-line no-constant-condition
while (true) {
Expand Down Expand Up @@ -86,7 +88,7 @@ export const startEventPolling = (queueId: number, eventId: number) => async (
}

// protection from inadvertent DDOS
await makeBackoffMachine();
await backoffMachine.wait();

if (e instanceof ApiError && e.code === 'BAD_EVENT_QUEUE_ID') {
// The event queue is too old or has been garbage collected.
Expand Down
3 changes: 2 additions & 1 deletion src/outbox/outboxActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ export const sendOutbox = () => async (dispatch: Dispatch, getState: GetState) =
return;
}
dispatch(toggleOutboxSending(true));
const backoffMachine = makeBackoffMachine();
while (!trySendMessages(dispatch, getState)) {
await makeBackoffMachine(); // eslint-disable-line no-await-in-loop
await backoffMachine.wait(); // eslint-disable-line no-await-in-loop
}
dispatch(toggleOutboxSending(false));
};
Expand Down
30 changes: 23 additions & 7 deletions src/utils/__tests__/makeBackoffMachine-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,31 @@
import { makeBackoffMachine } from '../async';

describe('makeBackoffMachine', () => {
test('first timeout is 0 seconds, then increase by 2 seconds', async () => {
const start = Date.now();
await makeBackoffMachine();
const duration = Date.now() - start;
expect(duration).toBeLessThan(1000);
test('timeouts are ~100ms, ~200ms, ~400ms, ~800ms', async () => {
const backoffMachine = makeBackoffMachine();

const start0 = Date.now();
await backoffMachine.wait();
const duration0 = Date.now() - start0;
expect(duration0).toBeGreaterThan(90);
expect(duration0).toBeLessThan(110);

const start1 = Date.now();
await backoffMachine.wait();
const duration1 = Date.now() - start1;
expect(duration1).toBeGreaterThan(190);
expect(duration1).toBeLessThan(210);

const start2 = Date.now();
await makeBackoffMachine();
await backoffMachine.wait();
const duration2 = Date.now() - start2;
expect(duration2).toBeGreaterThan(80);
expect(duration2).toBeGreaterThan(390);
expect(duration2).toBeLessThan(410);

const start3 = Date.now();
await backoffMachine.wait();
const duration3 = Date.now() - start3;
expect(duration3).toBeGreaterThan(790);
expect(duration3).toBeLessThan(810);
});
});
81 changes: 49 additions & 32 deletions src/utils/async.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,57 @@
/* @flow strict-local */
import { isClientError } from '../api/apiErrors';

/** The time at which we last started a timeout. */
let lastTimeoutTime = 0;
/**
* Makes a machine that can sleep for a timeout that, until a ceiling is reached,
* grows exponentially in duration with the number of sleeps completed, with a
* base of 2. The machine should be created before a loop starts, and .wait()
* should be called in each iteration of the loop. The machine should not be
* re-used after exiting the loop.
*
* The .numSleepsCompleted() and .timeElapsedSoFar() getters can be used in
* a control flow that breaks out of the loop after certain threshold. Note
* that .timeElapsedSoFar() will give the total time elapsed since the start
* of the first sleep, so this will include the time taken by other tasks
* like API requests.
*
* E.g., if firstDuration is 100 and durationCeiling is 10 * 1000 = 10000,
* the sequence is
*
* 100, 200, 400, 800, 1600, 3200, 6400, 10000, 10000, 10000, ...
*/
export const makeBackoffMachine = () => {
const firstDuration = 100;
const durationCeiling = 10 * 1000;
const base = 2;

/** The duration of the last timeout we started. */
let lastDuration = 0;
let startTime: number;
let numSleepsCompleted: number = 0;

const resetPeriod = 60 * 1000;
const maxDuration = 10 * 1000;
const incrementDuration = 100;
const exponent = 2;
return {
numSleepsCompleted() {
return numSleepsCompleted;
},

const chooseNextDuration = () => {
if (Date.now() > lastTimeoutTime + resetPeriod) {
return 0;
}
// prettier-ignore
return Math.min(maxDuration,
Math.max(incrementDuration,
exponent * lastDuration));
};
timeElapsedSoFar() {
return Date.now() - startTime;
},

// Renamed (NFC) from progressiveTimeout, to be immediately
// overwritten by makeBackoffMachine.
async wait(): Promise<void> {
if (startTime === undefined) {
startTime = Date.now();
}

/**
* Sleep for a timeout that progressively grows in duration.
*
* Starts at 0 on the first call. Grows initially by a small increment,
* then exponentially, up to a maximum.
*
* If the last call was over 60s ago, starts over at 0.
*/
export const makeBackoffMachine = (): Promise<void> => {
lastDuration = chooseNextDuration();
lastTimeoutTime = Date.now();
return new Promise(resolve => setTimeout(resolve, lastDuration));
const duration = Math.min(
// Should not exceed durationCeiling
durationCeiling,
firstDuration * base ** numSleepsCompleted,
);

await new Promise(resolve => setTimeout(resolve, duration));

numSleepsCompleted++;
},
};
};

/** Like setTimeout(..., 0), but returns a Promise of the result. */
Expand All @@ -56,6 +71,8 @@ export const sleep = (ms: number = 0): Promise<void> =>
*/
export async function tryUntilSuccessful<T>(func: () => Promise<T>): Promise<T> {
let clientError: Error | void;

const backoffMachine = makeBackoffMachine();
/* eslint-disable no-await-in-loop */
// eslint-disable-next-line no-constant-condition
while (true) {
Expand All @@ -66,7 +83,7 @@ export async function tryUntilSuccessful<T>(func: () => Promise<T>): Promise<T>
clientError = e;
break;
}
await makeBackoffMachine();
await backoffMachine.wait();
}
}
throw clientError;
Expand Down

0 comments on commit 126adf4

Please sign in to comment.