From 86ef36fa4415e90aeaea68e798aa99f857f51ebf Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 9 Dec 2024 15:39:25 -0700 Subject: [PATCH 1/6] Start extracting service policy boilerplate to common function We would like to use the Cockatiel library in our service classes to ensure that requests are retried using the circuit breaker pattern. Some of our service classes do this already, but we are copying and pasting the code around. This commit extracts some of the boilerplate code to a new function in the `@metamask/controller-utils` package, `createServicePolicy` so that we no longer have to do this. Because it would require an enormous amount of tests, this commit does not include a complete version of `createServicePolicy`, notably omitting `maxRetries` option as well as a way to handle slow requests; those will be added later. --- packages/controller-utils/CHANGELOG.md | 4 + packages/controller-utils/jest.config.js | 2 +- packages/controller-utils/package.json | 2 + .../src/create-service-policy.test.ts | 567 ++++++++++++++++++ .../src/create-service-policy.ts | 127 ++++ packages/controller-utils/src/index.test.ts | 75 +++ packages/controller-utils/src/index.ts | 1 + yarn.lock | 2 + 8 files changed, 779 insertions(+), 1 deletion(-) create mode 100644 packages/controller-utils/src/create-service-policy.test.ts create mode 100644 packages/controller-utils/src/create-service-policy.ts create mode 100644 packages/controller-utils/src/index.test.ts diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 5774eba9cab..313dc0fdd3e 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `createServicePolicy` function to assist with reducing boilerplate for service classes ([#5053](https://github.com/MetaMask/core/pull/5053)) + ## [11.4.5] ### Changed diff --git a/packages/controller-utils/jest.config.js b/packages/controller-utils/jest.config.js index 1d042ba6d40..746ae98e6f5 100644 --- a/packages/controller-utils/jest.config.js +++ b/packages/controller-utils/jest.config.js @@ -18,7 +18,7 @@ module.exports = merge(baseConfig, { coverageThreshold: { global: { branches: 78.12, - functions: 85.41, + functions: 84.61, lines: 87.3, statements: 86.5, }, diff --git a/packages/controller-utils/package.json b/packages/controller-utils/package.json index 8f67f9cfc2b..cb2ec2ee882 100644 --- a/packages/controller-utils/package.json +++ b/packages/controller-utils/package.json @@ -55,6 +55,7 @@ "@types/bn.js": "^5.1.5", "bignumber.js": "^9.1.2", "bn.js": "^5.2.1", + "cockatiel": "^3.1.2", "eth-ens-namehash": "^2.0.8", "fast-deep-equal": "^3.1.3" }, @@ -65,6 +66,7 @@ "deepmerge": "^4.2.2", "jest": "^27.5.1", "nock": "^13.3.1", + "sinon": "^9.2.4", "ts-jest": "^27.1.4", "typedoc": "^0.24.8", "typedoc-plugin-missing-exports": "^2.0.0", diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts new file mode 100644 index 00000000000..6a39f238024 --- /dev/null +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -0,0 +1,567 @@ +import { useFakeTimers } from 'sinon'; +import type { SinonFakeTimers } from 'sinon'; + +import { + createServicePolicy, + DEFAULT_MAX_CONSECUTIVE_FAILURES, + DEFAULT_MAX_RETRIES, +} from './create-service-policy'; + +describe('createServicePolicy', () => { + let clock: SinonFakeTimers; + + beforeEach(() => { + clock = useFakeTimers(); + }); + + afterEach(() => { + clock.restore(); + }); + + describe('wrapping a service that succeeds on the first try', () => { + it('returns a policy that returns what the service returns', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const policy = createServicePolicy(); + + const returnValue = await policy.execute(mockService); + + expect(returnValue).toStrictEqual({ some: 'data' }); + }); + + it('only calls the service once before returning', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const policy = createServicePolicy(); + + await policy.execute(mockService); + + expect(mockService).toHaveBeenCalledTimes(1); + }); + + it('does not call the onBreak callback, since the circuit never opens', async () => { + const mockService = jest.fn(() => ({ some: 'data' })); + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + await policy.execute(mockService); + + expect(onBreak).not.toHaveBeenCalled(); + }); + }); + + describe('wrapping a service that always fails', () => { + it(`calls the service a total of ${ + 1 + DEFAULT_MAX_RETRIES + } times, delaying each retry using a backoff formula`, async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy(); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(mockService).toHaveBeenCalledTimes(1 + DEFAULT_MAX_RETRIES); + }); + + it('calls the onRetry callback once per retry', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onRetry = jest.fn(); + const policy = createServicePolicy({ onRetry }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onRetry).toHaveBeenCalledTimes(DEFAULT_MAX_RETRIES); + }); + + describe(`using the default max number of consecutive failures (${DEFAULT_MAX_CONSECUTIVE_FAILURES})`, () => { + it('throws what the service throws', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy(); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow(error); + }); + + it('does not call the onBreak callback, since the max number of consecutive failures is never reached', async () => { + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).not.toHaveBeenCalled(); + }); + }); + + describe('using a custom max number of consecutive failures', () => { + describe('if the initial run + retries is less than the max number of consecutive failures', () => { + it('throws what the service throws', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow(error); + }); + + it('does not call the onBreak callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).not.toHaveBeenCalled(); + }); + }); + + describe('if the initial run + retries is equal to the max number of consecutive failures', () => { + it('throws what the service throws', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow(error); + }); + + it('calls the onBreak callback once with the error', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).toHaveBeenCalledTimes(1); + expect(onBreak).toHaveBeenCalledWith({ error }); + }); + + it('throws a BrokenCircuitError instead of whatever error the service produces if the service is executed again', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const firstExecution = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(firstExecution); + + const secondExecution = policy.execute(mockService); + await expect(secondExecution).rejects.toThrow( + new Error( + 'Execution prevented because the circuit breaker is open', + ), + ); + }); + }); + + describe('if the initial run + retries is greater than the max number of consecutive failures', () => { + it('throws a BrokenCircuitError instead of whatever error the service produces', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + await expect(promise).rejects.toThrow( + new Error( + 'Execution prevented because the circuit breaker is open', + ), + ); + }); + + it('calls the onBreak callback once with the error', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + const error = new Error('failure'); + const mockService = jest.fn(() => { + throw error; + }); + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).toHaveBeenCalledTimes(1); + expect(onBreak).toHaveBeenCalledWith({ error }); + }); + }); + }); + }); + + describe('wrapping a service that fails continuously and then succeeds on the final try', () => { + it(`calls the service a total of ${ + 1 + DEFAULT_MAX_RETRIES + } times, delaying each retry using a backoff formula`, async () => { + let invocationCounter = 0; + const mockService = jest.fn(() => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }); + const policy = createServicePolicy(); + // Each retry delay is randomized using a decorrelated jitter formula, so + // we need to make the delay times deterministic + jest.spyOn(Math, 'random').mockReturnValue(0); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(mockService).toHaveBeenCalledTimes(1 + DEFAULT_MAX_RETRIES); + }); + + it('calls the onRetry callback once per retry', async () => { + let invocationCounter = 0; + const mockService = jest.fn(() => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }); + const onRetry = jest.fn(); + const policy = createServicePolicy({ onRetry }); + // Each retry delay is randomized using a decorrelated jitter formula, so + // we need to make the delay times deterministic + jest.spyOn(Math, 'random').mockReturnValue(0); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onRetry).toHaveBeenCalledTimes(DEFAULT_MAX_RETRIES); + }); + + describe(`using the default max number of consecutive failures (${DEFAULT_MAX_CONSECUTIVE_FAILURES})`, () => { + it('returns what the service returns', async () => { + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + expect(await promise).toStrictEqual({ some: 'data' }); + }); + + it('does not call the onBreak callback, since the max number of consecutive failures is never reached', async () => { + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ onBreak }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onBreak).not.toHaveBeenCalled(); + }); + }); + + describe('using a custom max number of consecutive failures', () => { + describe('if the initial run + retries is less than the max number of consecutive failures', () => { + it('returns what the service returns', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + expect(await promise).toStrictEqual({ some: 'data' }); + }); + + it('does not call the onBreak callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 2; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onBreak).not.toHaveBeenCalled(); + }); + }); + + describe('if the initial run + retries is equal to the max number of consecutive failures', () => { + it('returns what the service returns', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + let invocationCounter = 0; + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw new Error('failure'); + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + + expect(await promise).toStrictEqual({ some: 'data' }); + }); + + it('does not call the onBreak callback', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES + 1; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await promise; + + expect(onBreak).not.toHaveBeenCalled(); + }); + }); + + describe('if the initial run + retries is greater than the max number of consecutive failures', () => { + it('throws a BrokenCircuitError before the service can succeed', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await expect(promise).rejects.toThrow( + new Error( + 'Execution prevented because the circuit breaker is open', + ), + ); + }); + + it('calls the onBreak callback once with the error', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const onBreak = jest.fn(); + const policy = createServicePolicy({ + maxConsecutiveFailures, + onBreak, + }); + + const promise = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(promise); + + expect(onBreak).toHaveBeenCalledTimes(1); + expect(onBreak).toHaveBeenCalledWith({ error }); + }); + }); + }); + }); +}); + +/** + * Some tests involve a rejected promise that is not necessarily the focus of + * the test. In these cases we don't want to ignore the error in case the + * promise _isn't_ rejected, but we don't want to highlight the assertion, + * either. + * + * @param promise - A promise that rejects. + */ +async function ignoreRejection(promise: Promise) { + await expect(promise).rejects.toThrow(expect.any(Error)); +} diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts new file mode 100644 index 00000000000..bdfc0105e64 --- /dev/null +++ b/packages/controller-utils/src/create-service-policy.ts @@ -0,0 +1,127 @@ +import { + circuitBreaker, + ConsecutiveBreaker, + ExponentialBackoff, + handleAll, + retry, + wrap, +} from 'cockatiel'; +import type { IPolicy } from 'cockatiel'; + +export type { IPolicy as IServicePolicy }; + +/** + * The maximum number of times that a failing service should be re-run before + * giving up. + */ +export const DEFAULT_MAX_RETRIES = 3; + +/** + * The maximum number of times that the service is allowed to fail before + * pausing further retries. + */ +export const DEFAULT_MAX_CONSECUTIVE_FAILURES = (1 + DEFAULT_MAX_RETRIES) * 3; + +/** + * The default length of time (in milliseconds) to temporarily pause retries of + * the service after enough consecutive failures. + */ +export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; + +/** + * Constructs an object exposing an `execute` method which, given a function — + * hereafter called the "service" — will retry that service with ever increasing + * delays until it succeeds. If the policy detects too many consecutive + * failures, it will block further retries until a designated time period has + * passed; this particular behavior is primarily designed for services that wrap + * API calls so as not to make needless HTTP requests when the API is down and + * to be able to recover when the API comes back up. In addition, hooks allow + * for responding to certain events, one of which can be used to detect when an + * HTTP request is performing slowly. + * + * Internally, this function makes use of the retry and circuit breaker policies + * from the [Cockatiel](https://www.npmjs.com/package/cockatiel) library; see + * there for more. + * + * @param options - The options to this function. + * @param options.maxConsecutiveFailures - The maximum number of times that + * the action is allowed to fail before pausing further retries. Defaults to 12. + * @param options.onBreak - A function which is called when the action fails too + * many times in a row (specifically, more than `maxConsecutiveFailures`). + * @param options.onRetry - A function which will be called the moment the + * policy kicks off a timer to re-run the function passed to the policy. This + * is primarily useful in tests where we are mocking timers. + * @returns The service policy. + * @example + * This function is designed to be used in the context of a service class like + * this: + * ``` ts + * class Service { + * constructor() { + * this.#policy = createServicePolicy({ + * maxConsecutiveFailures: 3, + * onBreak: () => { + * console.log('Circuit broke'); + * }, + * }); + * } + * + * async fetch() { + * return await this.#policy.execute(async () => { + * const response = await fetch('https://some/url'); + * return await response.json(); + * }); + * } + * } + * ``` + */ +export function createServicePolicy({ + maxConsecutiveFailures = DEFAULT_MAX_CONSECUTIVE_FAILURES, + onBreak = () => { + // do nothing + }, + onRetry = () => { + // do nothing + }, +}: { + maxConsecutiveFailures?: number; + onBreak?: () => void; + onRetry?: () => void; +} = {}): IPolicy { + const retryPolicy = retry(handleAll, { + // Note that although the option here is called "max attempts", it's really + // maximum number of *retries* (attempts past the initial attempt). + maxAttempts: DEFAULT_MAX_RETRIES, + // Retries of the service will be executed following ever increasing delays, + // determined by a backoff formula. + backoff: new ExponentialBackoff(), + }); + + const circuitBreakerPolicy = circuitBreaker(handleAll, { + // While the circuit is open, any additional invocations of the service + // passed to the policy (either via automatic retries or by manually + // executing the policy again) will result in a BrokenCircuitError. This + // will remain the case until `circuitBreakDuration` passes, after which the + // service will be allowed to run again. If the service succeeds, the + // circuit will close, otherwise it will remain open. + halfOpenAfter: DEFAULT_CIRCUIT_BREAK_DURATION, + breaker: new ConsecutiveBreaker(maxConsecutiveFailures), + }); + + // The `onBreak` callback will be called if the service consistently throws + // for as many times as exceeds the maximum consecutive number of failures. + // Combined with the retry policy, this can happen if: + // - `maxRetries` > `maxConsecutiveFailures` and the policy is executed once + // - `maxRetries` <= `maxConsecutiveFailures` but the policy is executed + // multiple times, enough for the total number of retries to exceed + // `maxConsecutiveFailures` + circuitBreakerPolicy.onBreak(onBreak); + + // The `onRetryPolicy` callback will be called each time the service is + // invoked (including retries). + retryPolicy.onRetry(onRetry); + + // The retry policy really retries the circuit breaker policy, which invokes + // the service. + return wrap(retryPolicy, circuitBreakerPolicy); +} diff --git a/packages/controller-utils/src/index.test.ts b/packages/controller-utils/src/index.test.ts new file mode 100644 index 00000000000..61ef841826f --- /dev/null +++ b/packages/controller-utils/src/index.test.ts @@ -0,0 +1,75 @@ +import * as allExports from '.'; + +describe('@metamask/controller-utils', () => { + it('has expected JavaScript exports', () => { + expect(Object.keys(allExports)).toMatchInlineSnapshot(` + Array [ + "createServicePolicy", + "BNToHex", + "convertHexToDecimal", + "fetchWithErrorHandling", + "fractionBN", + "fromHex", + "getBuyURL", + "gweiDecToWEIBN", + "handleFetch", + "hexToBN", + "hexToText", + "isNonEmptyArray", + "isPlainObject", + "isSafeChainId", + "isSafeDynamicKey", + "isSmartContractCode", + "isValidJson", + "isValidHexAddress", + "normalizeEnsName", + "query", + "safelyExecute", + "safelyExecuteWithTimeout", + "successfulFetch", + "timeoutFetch", + "toChecksumHexAddress", + "toHex", + "weiHexToGweiDec", + "isEqualCaseInsensitive", + "RPC", + "FALL_BACK_VS_CURRENCY", + "IPFS_DEFAULT_GATEWAY_URL", + "GANACHE_CHAIN_ID", + "MAX_SAFE_CHAIN_ID", + "ERC721", + "ERC1155", + "ERC20", + "ERC721_INTERFACE_ID", + "ERC721_METADATA_INTERFACE_ID", + "ERC721_ENUMERABLE_INTERFACE_ID", + "ERC1155_INTERFACE_ID", + "ERC1155_METADATA_URI_INTERFACE_ID", + "ERC1155_TOKEN_RECEIVER_INTERFACE_ID", + "GWEI", + "ASSET_TYPES", + "TESTNET_TICKER_SYMBOLS", + "BUILT_IN_NETWORKS", + "OPENSEA_PROXY_URL", + "NFT_API_BASE_URL", + "NFT_API_VERSION", + "NFT_API_TIMEOUT", + "ORIGIN_METAMASK", + "ApprovalType", + "CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP", + "InfuraNetworkType", + "NetworkType", + "isNetworkType", + "isInfuraNetworkType", + "BuiltInNetworkName", + "ChainId", + "NetworksTicker", + "BlockExplorerUrl", + "NetworkNickname", + "parseDomainParts", + "isValidSIWEOrigin", + "detectSIWE", + ] + `); + }); +}); diff --git a/packages/controller-utils/src/index.ts b/packages/controller-utils/src/index.ts index 3d35d62c0a0..b3bd8821e12 100644 --- a/packages/controller-utils/src/index.ts +++ b/packages/controller-utils/src/index.ts @@ -1,3 +1,4 @@ +export { createServicePolicy } from './create-service-policy'; export * from './constants'; export type { NonEmptyArray } from './util'; export { diff --git a/yarn.lock b/yarn.lock index aa94150c2f1..655e133e455 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2550,11 +2550,13 @@ __metadata: "@types/jest": "npm:^27.4.1" bignumber.js: "npm:^9.1.2" bn.js: "npm:^5.2.1" + cockatiel: "npm:^3.1.2" deepmerge: "npm:^4.2.2" eth-ens-namehash: "npm:^2.0.8" fast-deep-equal: "npm:^3.1.3" jest: "npm:^27.5.1" nock: "npm:^13.3.1" + sinon: "npm:^9.2.4" ts-jest: "npm:^27.1.4" typedoc: "npm:^0.24.8" typedoc-plugin-missing-exports: "npm:^2.0.0" From 073f5dc19ae9ad125bc40674831a3b8581c5fa68 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 13 Jan 2025 16:15:08 -0700 Subject: [PATCH 2/6] No need to mock Math.random --- packages/controller-utils/src/create-service-policy.test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts index 6a39f238024..3041c93b2bb 100644 --- a/packages/controller-utils/src/create-service-policy.test.ts +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -300,9 +300,6 @@ describe('createServicePolicy', () => { throw new Error('failure'); }); const policy = createServicePolicy(); - // Each retry delay is randomized using a decorrelated jitter formula, so - // we need to make the delay times deterministic - jest.spyOn(Math, 'random').mockReturnValue(0); const promise = policy.execute(mockService); // It's safe not to await this promise; adding it to the promise queue @@ -325,9 +322,6 @@ describe('createServicePolicy', () => { }); const onRetry = jest.fn(); const policy = createServicePolicy({ onRetry }); - // Each retry delay is randomized using a decorrelated jitter formula, so - // we need to make the delay times deterministic - jest.spyOn(Math, 'random').mockReturnValue(0); const promise = policy.execute(mockService); // It's safe not to await this promise; adding it to the promise queue From d34010234078f40441a5c3df6ba0713ef3a0aa18 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 13 Jan 2025 16:18:27 -0700 Subject: [PATCH 3/6] Add a missing test --- .../src/create-service-policy.test.ts | 28 +++++++++++++++++++ .../src/create-service-policy.ts | 13 +++++---- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts index 3041c93b2bb..c3d322a4c5e 100644 --- a/packages/controller-utils/src/create-service-policy.test.ts +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -3,6 +3,7 @@ import type { SinonFakeTimers } from 'sinon'; import { createServicePolicy, + DEFAULT_CIRCUIT_BREAK_DURATION, DEFAULT_MAX_CONSECUTIVE_FAILURES, DEFAULT_MAX_RETRIES, } from './create-service-policy'; @@ -543,6 +544,33 @@ describe('createServicePolicy', () => { expect(onBreak).toHaveBeenCalledTimes(1); expect(onBreak).toHaveBeenCalledWith({ error }); }); + + it('returns what the service returns if it is successfully called again after the default circuit break duration has elapsed', async () => { + const maxConsecutiveFailures = DEFAULT_MAX_RETRIES; + let invocationCounter = 0; + const error = new Error('failure'); + const mockService = () => { + invocationCounter += 1; + if (invocationCounter === DEFAULT_MAX_RETRIES + 1) { + return { some: 'data' }; + } + throw error; + }; + const policy = createServicePolicy({ + maxConsecutiveFailures, + }); + + const firstExecution = policy.execute(mockService); + // It's safe not to await this promise; adding it to the promise + // queue is enough to prevent this test from running indefinitely. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + clock.runAllAsync(); + await ignoreRejection(firstExecution); + clock.tick(DEFAULT_CIRCUIT_BREAK_DURATION); + const result = await policy.execute(mockService); + + expect(result).toStrictEqual({ some: 'data' }); + }); }); }); }); diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts index bdfc0105e64..dd36c51f670 100644 --- a/packages/controller-utils/src/create-service-policy.ts +++ b/packages/controller-utils/src/create-service-policy.ts @@ -101,9 +101,9 @@ export function createServicePolicy({ // While the circuit is open, any additional invocations of the service // passed to the policy (either via automatic retries or by manually // executing the policy again) will result in a BrokenCircuitError. This - // will remain the case until `circuitBreakDuration` passes, after which the - // service will be allowed to run again. If the service succeeds, the - // circuit will close, otherwise it will remain open. + // will remain the case until the default circuit break duration passes, + // after which the service will be allowed to run again. If the service + // succeeds, the circuit will close, otherwise it will remain open. halfOpenAfter: DEFAULT_CIRCUIT_BREAK_DURATION, breaker: new ConsecutiveBreaker(maxConsecutiveFailures), }); @@ -111,9 +111,10 @@ export function createServicePolicy({ // The `onBreak` callback will be called if the service consistently throws // for as many times as exceeds the maximum consecutive number of failures. // Combined with the retry policy, this can happen if: - // - `maxRetries` > `maxConsecutiveFailures` and the policy is executed once - // - `maxRetries` <= `maxConsecutiveFailures` but the policy is executed - // multiple times, enough for the total number of retries to exceed + // - `maxConsecutiveFailures` < the default max retries (3) and the policy is + // executed once + // - `maxConsecutiveFailures` >= the default max retries (3) but the policy is + // executed multiple times, enough for the total number of retries to exceed // `maxConsecutiveFailures` circuitBreakerPolicy.onBreak(onBreak); From 3f682530de58e6ba3f71495ea058bed8190945a3 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 13 Jan 2025 16:26:39 -0700 Subject: [PATCH 4/6] Tweak comments --- .../src/create-service-policy.test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/controller-utils/src/create-service-policy.test.ts b/packages/controller-utils/src/create-service-policy.test.ts index c3d322a4c5e..320141586b2 100644 --- a/packages/controller-utils/src/create-service-policy.test.ts +++ b/packages/controller-utils/src/create-service-policy.test.ts @@ -60,8 +60,8 @@ describe('createServicePolicy', () => { const policy = createServicePolicy(); const promise = policy.execute(mockService); - // It's safe not to await this promise; adding it to the promise queue - // is enough to prevent this test from running indefinitely. + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. // eslint-disable-next-line @typescript-eslint/no-floating-promises clock.runAllAsync(); await ignoreRejection(promise); @@ -78,8 +78,8 @@ describe('createServicePolicy', () => { const policy = createServicePolicy({ onRetry }); const promise = policy.execute(mockService); - // It's safe not to await this promise; adding it to the promise queue - // is enough to prevent this test from running indefinitely. + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. // eslint-disable-next-line @typescript-eslint/no-floating-promises clock.runAllAsync(); await ignoreRejection(promise); @@ -303,8 +303,8 @@ describe('createServicePolicy', () => { const policy = createServicePolicy(); const promise = policy.execute(mockService); - // It's safe not to await this promise; adding it to the promise queue - // is enough to prevent this test from running indefinitely. + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. // eslint-disable-next-line @typescript-eslint/no-floating-promises clock.runAllAsync(); await promise; @@ -325,8 +325,8 @@ describe('createServicePolicy', () => { const policy = createServicePolicy({ onRetry }); const promise = policy.execute(mockService); - // It's safe not to await this promise; adding it to the promise queue - // is enough to prevent this test from running indefinitely. + // It's safe not to await this promise; adding it to the promise queue is + // enough to prevent this test from running indefinitely. // eslint-disable-next-line @typescript-eslint/no-floating-promises clock.runAllAsync(); await promise; @@ -561,8 +561,8 @@ describe('createServicePolicy', () => { }); const firstExecution = policy.execute(mockService); - // It's safe not to await this promise; adding it to the promise - // queue is enough to prevent this test from running indefinitely. + // It's safe not to await this promise; adding it to the promise queue + // is enough to prevent this test from running indefinitely. // eslint-disable-next-line @typescript-eslint/no-floating-promises clock.runAllAsync(); await ignoreRejection(firstExecution); From ffa74d82974fd4444c97da6d9287a1e017c0e498 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 13 Jan 2025 16:29:38 -0700 Subject: [PATCH 5/6] Tweak JSDoc --- packages/controller-utils/src/create-service-policy.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts index dd36c51f670..aae078d076e 100644 --- a/packages/controller-utils/src/create-service-policy.ts +++ b/packages/controller-utils/src/create-service-policy.ts @@ -45,9 +45,9 @@ export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; * * @param options - The options to this function. * @param options.maxConsecutiveFailures - The maximum number of times that - * the action is allowed to fail before pausing further retries. Defaults to 12. - * @param options.onBreak - A function which is called when the action fails too - * many times in a row (specifically, more than `maxConsecutiveFailures`). + * the service is allowed to fail before pausing further retries. Defaults to 12. + * @param options.onBreak - A function which is called when the service fails + * too many times in a row (specifically, more than `maxConsecutiveFailures`). * @param options.onRetry - A function which will be called the moment the * policy kicks off a timer to re-run the function passed to the policy. This * is primarily useful in tests where we are mocking timers. From a669e347fb09e7617016dd7c55eb026a7bfe4b3b Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Mon, 13 Jan 2025 16:31:40 -0700 Subject: [PATCH 6/6] Tweak JSDoc some more --- packages/controller-utils/src/create-service-policy.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/controller-utils/src/create-service-policy.ts b/packages/controller-utils/src/create-service-policy.ts index aae078d076e..40651df3c5f 100644 --- a/packages/controller-utils/src/create-service-policy.ts +++ b/packages/controller-utils/src/create-service-policy.ts @@ -44,13 +44,13 @@ export const DEFAULT_CIRCUIT_BREAK_DURATION = 30 * 60 * 1000; * there for more. * * @param options - The options to this function. - * @param options.maxConsecutiveFailures - The maximum number of times that - * the service is allowed to fail before pausing further retries. Defaults to 12. + * @param options.maxConsecutiveFailures - The maximum number of times that the + * service is allowed to fail before pausing further retries. Defaults to 12. * @param options.onBreak - A function which is called when the service fails * too many times in a row (specifically, more than `maxConsecutiveFailures`). * @param options.onRetry - A function which will be called the moment the - * policy kicks off a timer to re-run the function passed to the policy. This - * is primarily useful in tests where we are mocking timers. + * policy kicks off a timer to re-run the function passed to the policy. This is + * primarily useful in tests where we are mocking timers. * @returns The service policy. * @example * This function is designed to be used in the context of a service class like