From 19b86a8632d8bb45d997520a9a8fdf7f9f427fd8 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Sun, 21 Jul 2024 21:23:28 -0700 Subject: [PATCH] refactor: store backoff in circuit breaker state --- package-lock.json | 16 +++++------ package.json | 2 +- src/CircuitBreakerPolicy.test.ts | 46 ++++++++++++++++++++------------ src/CircuitBreakerPolicy.ts | 40 ++++++++++++++++++--------- src/backoff/Backoff.ts | 3 +-- src/backoff/IterableBackoff.ts | 2 +- 6 files changed, 68 insertions(+), 41 deletions(-) diff --git a/package-lock.json b/package-lock.json index fe96f88..113c494 100644 --- a/package-lock.json +++ b/package-lock.json @@ -29,7 +29,7 @@ "sinon": "^14.0.0", "sinon-chai": "^3.7.0", "source-map-support": "^0.5.21", - "typescript": "^4.7.4" + "typescript": "^5.5.3" }, "engines": { "node": ">=16" @@ -4124,16 +4124,16 @@ } }, "node_modules/typescript": { - "version": "4.7.4", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.7.4.tgz", - "integrity": "sha512-C0WQT0gezHuw6AdY1M2jxUO83Rjf0HP7Sk1DtXj6j1EwkQNZrHAg2XPWlq62oqEhYvONq5pkC2Y9oPljWToLmQ==", + "version": "5.5.3", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.5.3.tgz", + "integrity": "sha512-/hreyEujaB0w76zKo6717l3L0o/qEUtRgdvUBvlkhoWeOVMjMuHNHk0BRBzikzuGDqNmPQbg5ifMEqsHLiIUcQ==", "dev": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" }, "engines": { - "node": ">=4.2.0" + "node": ">=14.17" } }, "node_modules/unified": { @@ -7825,9 +7825,9 @@ } }, "typescript": { - "version": "4.7.4", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.7.4.tgz", - "integrity": "sha512-C0WQT0gezHuw6AdY1M2jxUO83Rjf0HP7Sk1DtXj6j1EwkQNZrHAg2XPWlq62oqEhYvONq5pkC2Y9oPljWToLmQ==", + "version": "5.5.3", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.5.3.tgz", + "integrity": "sha512-/hreyEujaB0w76zKo6717l3L0o/qEUtRgdvUBvlkhoWeOVMjMuHNHk0BRBzikzuGDqNmPQbg5ifMEqsHLiIUcQ==", "dev": true }, "unified": { diff --git a/package.json b/package.json index e348361..16eac0c 100644 --- a/package.json +++ b/package.json @@ -80,7 +80,7 @@ "sinon": "^14.0.0", "sinon-chai": "^3.7.0", "source-map-support": "^0.5.21", - "typescript": "^4.7.4" + "typescript": "^5.5.3" }, "prettier": { "printWidth": 100, diff --git a/src/CircuitBreakerPolicy.test.ts b/src/CircuitBreakerPolicy.test.ts index 35204e7..85888dc 100644 --- a/src/CircuitBreakerPolicy.test.ts +++ b/src/CircuitBreakerPolicy.test.ts @@ -1,13 +1,18 @@ import { expect } from 'chai'; import { SinonFakeTimers, SinonStub, stub, useFakeTimers } from 'sinon'; import { promisify } from 'util'; +import { IBackoffFactory } from './backoff/Backoff'; +import { IterableBackoff } from './backoff/IterableBackoff'; import { ConsecutiveBreaker } from './breaker/Breaker'; -import { CircuitBreakerPolicy, CircuitState } from './CircuitBreakerPolicy'; +import { + CircuitBreakerPolicy, + CircuitState, + IHalfOpenAfterBackoffContext, +} from './CircuitBreakerPolicy'; import { abortedSignal } from './common/abort'; import { BrokenCircuitError, TaskCancelledError } from './errors/Errors'; import { IsolatedCircuitError } from './errors/IsolatedCircuitError'; import { circuitBreaker, handleAll, handleType } from './Policy'; -import { IterableBackoff } from './backoff/IterableBackoff'; class MyException extends Error {} @@ -115,8 +120,17 @@ describe('CircuitBreakerPolicy', () => { }); it('resets the backoff when closing the circuit', async () => { + let args: { duration: number; attempt: number }[] = []; p = circuitBreaker(handleType(MyException), { - halfOpenAfter: new IterableBackoff([1000, 2000]), + halfOpenAfter: new (class MyBreaker implements IBackoffFactory { + constructor(public readonly duration: number) {} + + next(context: IHalfOpenAfterBackoffContext) { + args.push({ duration: this.duration + 1, attempt: context.attempt }); + expect('error' in context.result).to.be.true; + return new MyBreaker(this.duration + 1); + } + })(0), breaker: new ConsecutiveBreaker(2), }); p.onReset(onReset); @@ -124,25 +138,23 @@ describe('CircuitBreakerPolicy', () => { await openBreaker(); - clock.tick(1000); + expect(args).to.deep.equal([{ duration: 1, attempt: 1 }]); + clock.tick(args.pop()!.duration); - const halfOpenTest1 = p.execute(stub().resolves(42)); - expect(p.state).to.equal(CircuitState.HalfOpen); - expect(onHalfOpen).calledOnce; - expect(await halfOpenTest1).to.equal(42); - expect(p.state).to.equal(CircuitState.Closed); - expect(onReset).calledOnce; + await expect(p.execute(stub().throws(new MyException()))).to.be.rejectedWith(MyException); + expect(args).to.deep.equal([{ duration: 2, attempt: 2 }]); + clock.tick(args.pop()!.duration); + + await p.execute(stub().resolves(42)); + expect(args).to.be.empty; await openBreaker(); - clock.tick(1000); + expect(args).to.deep.equal([{ duration: 1, attempt: 1 }]); + clock.tick(args.pop()!.duration); - const halfOpenTest2 = p.execute(stub().resolves(42)); - expect(p.state).to.equal(CircuitState.HalfOpen); - expect(onHalfOpen).calledTwice; - expect(await halfOpenTest2).to.equal(42); - expect(p.state).to.equal(CircuitState.Closed); - expect(onReset).calledTwice; + await p.execute(stub().resolves(42)); + expect(args).to.be.empty; }); it('dedupes half-open tests', async () => { diff --git a/src/CircuitBreakerPolicy.ts b/src/CircuitBreakerPolicy.ts index feb13c4..39c1039 100644 --- a/src/CircuitBreakerPolicy.ts +++ b/src/CircuitBreakerPolicy.ts @@ -61,8 +61,18 @@ export interface ICircuitBreakerOptions { type InnerState = | { value: CircuitState.Closed } | { value: CircuitState.Isolated; counters: number } - | { value: CircuitState.Open; openedAt: number } - | { value: CircuitState.HalfOpen; test: Promise }; + | { + value: CircuitState.Open; + openedAt: number; + attemptNo: number; + backoff: IBackoff; + } + | { + value: CircuitState.HalfOpen; + test: Promise; + attemptNo: number; + backoff: IBackoff; + }; export class CircuitBreakerPolicy implements IPolicy { declare readonly _altReturn: never; @@ -74,8 +84,6 @@ export class CircuitBreakerPolicy implements IPolicy { private readonly halfOpenAfterBackoffFactory: IBackoffFactory; private innerLastFailure?: FailureReason; private innerState: InnerState = { value: CircuitState.Closed }; - private openEnteredCount = 0; - private halfOpenAfterBackof: IBackoff | undefined; /** * Event emitted when the circuit breaker opens. @@ -198,11 +206,16 @@ export class CircuitBreakerPolicy implements IPolicy { return this.execute(fn); case CircuitState.Open: - if (Date.now() - state.openedAt < (this.halfOpenAfterBackof?.duration ?? 0)) { + if (Date.now() - state.openedAt < state.backoff.duration) { throw new BrokenCircuitError(); } const test = this.halfOpen(fn, signal); - this.innerState = { value: CircuitState.HalfOpen, test }; + this.innerState = { + value: CircuitState.HalfOpen, + test, + backoff: state.backoff, + attemptNo: state.attemptNo + 1, + }; this.stateChangeEmitter.emit(CircuitState.HalfOpen); return test; @@ -245,12 +258,17 @@ export class CircuitBreakerPolicy implements IPolicy { return; } - this.innerState = { value: CircuitState.Open, openedAt: Date.now() }; + const attemptNo = + this.innerState.value === CircuitState.HalfOpen ? this.innerState.attemptNo : 1; + const context = { attempt: attemptNo, result: reason, signal }; + const backoff = + this.innerState.value === CircuitState.HalfOpen + ? this.innerState.backoff.next(context) + : this.halfOpenAfterBackoffFactory.next(context); + + this.innerState = { value: CircuitState.Open, openedAt: Date.now(), backoff, attemptNo }; this.breakEmitter.emit(reason); this.stateChangeEmitter.emit(CircuitState.Open); - const context = { attempt: ++this.openEnteredCount, result: reason, signal }; - this.halfOpenAfterBackof = - this.halfOpenAfterBackof?.next(context) ?? this.halfOpenAfterBackoffFactory.next(context); } private close() { @@ -258,8 +276,6 @@ export class CircuitBreakerPolicy implements IPolicy { this.innerState = { value: CircuitState.Closed }; this.resetEmitter.emit(); this.stateChangeEmitter.emit(CircuitState.Closed); - this.halfOpenAfterBackof = undefined; - this.openEnteredCount = 0; } } } diff --git a/src/backoff/Backoff.ts b/src/backoff/Backoff.ts index a6fffbd..5e98da5 100644 --- a/src/backoff/Backoff.ts +++ b/src/backoff/Backoff.ts @@ -3,8 +3,7 @@ */ export interface IBackoffFactory { /** - * Returns the first backoff duration. Can return "undefined" to signal - * that we should not back off. + * Returns the first backoff duration. */ next(context: T): IBackoff; } diff --git a/src/backoff/IterableBackoff.ts b/src/backoff/IterableBackoff.ts index 44c997d..91d5e02 100644 --- a/src/backoff/IterableBackoff.ts +++ b/src/backoff/IterableBackoff.ts @@ -9,7 +9,7 @@ export class IterableBackoff implements IBackoffFactory { /** * @inheritdoc */ - public next() { + public next(_context: unknown) { return instance(this.durations, 0); } }