From 7e0881d9116289456b2f8e5c312c01e64921f8b3 Mon Sep 17 00:00:00 2001 From: Soma Lucz Date: Sun, 15 Mar 2020 17:04:33 +0100 Subject: [PATCH] refactor: Better errors in BackoffStrategyFactory Closes #161. --- README.md | 8 +- .../proactive/timeoutPolicy/timeoutPolicy.ts | 2 +- .../retryPolicy/backoffStrategyFactory.ts | 28 ++ test/specs/backoffStrategyFactory.test.ts | 244 +++++++++++++++++- test/specs/timeoutPolicy.test.ts | 11 + 5 files changed, 285 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index f40136b..76ba351 100644 --- a/README.md +++ b/README.md @@ -340,11 +340,11 @@ policy.waitBeforeRetry(BackoffStrategyFactory.exponentialBackoff(100, false, 3)) // 0 100 300 900 2700 … policy.waitBeforeRetry(BackoffStrategyFactory.exponentialBackoff(100, true, 3)); -// wait for a [random between 0-100, inclusive] ms before each retry -policy.waitBeforeRetry(BackoffStrategyFactory.jitteredBackoff(0, 100)); +// wait for a [random between 1-100, inclusive] ms before each retry +policy.waitBeforeRetry(BackoffStrategyFactory.jitteredBackoff(1, 100)); -// retry immediately for the first time, then wait for a [random between 0-100, inclusive] ms before each retry -policy.waitBeforeRetry(BackoffStrategyFactory.jitteredBackoff(0, 100, true)); +// retry immediately for the first time, then wait for a [random between 1-100, inclusive] ms before each retry +policy.waitBeforeRetry(BackoffStrategyFactory.jitteredBackoff(1, 100, true)); ``` For using `jitteredBackoff` in Node.js environments, you will need to inject a Node.js-based entropy source into the default RandomGenerator ([@diplomatiq/crypto-random](https://github.com/Diplomatiq/crypto-random) requires `window.crypto.getRandomValues` to be available by default). Create the following in your project: diff --git a/src/policies/proactive/timeoutPolicy/timeoutPolicy.ts b/src/policies/proactive/timeoutPolicy/timeoutPolicy.ts index 34b95fe..8b9a8d0 100644 --- a/src/policies/proactive/timeoutPolicy/timeoutPolicy.ts +++ b/src/policies/proactive/timeoutPolicy/timeoutPolicy.ts @@ -16,7 +16,7 @@ export class TimeoutPolicy extends ProactivePolicy { throw new Error('timeoutMs must be integer'); } - if (timeoutMs < 0) { + if (timeoutMs <= 0) { throw new Error('timeoutMs must be greater than 0'); } diff --git a/src/policies/reactive/retryPolicy/backoffStrategyFactory.ts b/src/policies/reactive/retryPolicy/backoffStrategyFactory.ts index 9bd8576..fb6e6b9 100644 --- a/src/policies/reactive/retryPolicy/backoffStrategyFactory.ts +++ b/src/policies/reactive/retryPolicy/backoffStrategyFactory.ts @@ -3,12 +3,16 @@ import { RandomGenerator } from '../../../interfaces/randomGenerator'; export class BackoffStrategyFactory { public static constantBackoff(delayMs: number, fastFirst = false): (currentRetryCount: number) => number { + this.validateNumericArgument(delayMs, 'delayMs'); + return fastFirst ? (currentRetryCount: number): number => (currentRetryCount === 1 ? 0 : delayMs) : (): number => delayMs; } public static linearBackoff(delayMs: number, fastFirst = false): (currentRetryCount: number) => number { + this.validateNumericArgument(delayMs, 'delayMs'); + return fastFirst ? (currentRetryCount: number): number => delayMs * (currentRetryCount - 1) : (currentRetryCount: number): number => delayMs * currentRetryCount; @@ -19,6 +23,9 @@ export class BackoffStrategyFactory { fastFirst = false, base = 2, ): (currentRetryCount: number) => number { + this.validateNumericArgument(delayMs, 'delayMs'); + this.validateNumericArgument(base, 'base'); + return fastFirst ? (currentRetryCount: number): number => currentRetryCount === 1 ? 0 : delayMs * base ** (currentRetryCount - 2) @@ -31,6 +38,13 @@ export class BackoffStrategyFactory { fastFirst = false, randomGenerator: RandomGenerator = new DefaultRandomGenerator(), ): (currentRetryCount: number) => Promise { + this.validateNumericArgument(minDelayMs, 'minDelayMs'); + this.validateNumericArgument(maxDelayMs, 'maxDelayMs'); + + if (maxDelayMs <= minDelayMs) { + throw new Error('maxDelayMs must be greater than minDelayMs'); + } + return fastFirst ? async (currentRetryCount: number): Promise => { if (currentRetryCount === 1) { @@ -44,4 +58,18 @@ export class BackoffStrategyFactory { return ms; }; } + + private static validateNumericArgument(arg: number, name: string): void { + if (!Number.isInteger(arg)) { + throw new Error(`${name} must be integer`); + } + + if (arg <= 0) { + throw new Error(`${name} must be greater than 0`); + } + + if (!Number.isSafeInteger(arg)) { + throw new Error(`${name} must be less than or equal to 2^53 - 1`); + } + } } diff --git a/test/specs/backoffStrategyFactory.test.ts b/test/specs/backoffStrategyFactory.test.ts index cf3e942..40dd134 100644 --- a/test/specs/backoffStrategyFactory.test.ts +++ b/test/specs/backoffStrategyFactory.test.ts @@ -23,6 +23,38 @@ describe('BackoffStrategyFactory', (): void => { expect(strategy(4)).to.equal(100); expect(strategy(5)).to.equal(100); }); + + it('should throw error when setting delayMs to 0', (): void => { + try { + BackoffStrategyFactory.constantBackoff(0); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be greater than 0'); + } + }); + + it('should throw error when setting delayMs to <0', (): void => { + try { + BackoffStrategyFactory.constantBackoff(-1); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be greater than 0'); + } + }); + + it('should throw error when setting delayMs to a non-integer', (): void => { + try { + BackoffStrategyFactory.constantBackoff(0.1); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be integer'); + } + }); + + it('should throw error when setting delayMs to a non-safe integer', (): void => { + try { + BackoffStrategyFactory.constantBackoff(2 ** 53); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be less than or equal to 2^53 - 1'); + } + }); }); describe('linearBackoff', (): void => { @@ -43,6 +75,38 @@ describe('BackoffStrategyFactory', (): void => { expect(strategy(4)).to.equal(300); expect(strategy(5)).to.equal(400); }); + + it('should throw error when setting delayMs to 0', (): void => { + try { + BackoffStrategyFactory.linearBackoff(0); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be greater than 0'); + } + }); + + it('should throw error when setting delayMs to <0', (): void => { + try { + BackoffStrategyFactory.linearBackoff(-1); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be greater than 0'); + } + }); + + it('should throw error when setting delayMs to a non-integer', (): void => { + try { + BackoffStrategyFactory.linearBackoff(0.1); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be integer'); + } + }); + + it('should throw error when setting delayMs to a non-safe integer', (): void => { + try { + BackoffStrategyFactory.linearBackoff(2 ** 53); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be less than or equal to 2^53 - 1'); + } + }); }); describe('exponentialBackoff', (): void => { @@ -81,13 +145,77 @@ describe('BackoffStrategyFactory', (): void => { expect(strategy(4)).to.equal(900); expect(strategy(5)).to.equal(2700); }); + + it('should throw error when setting delayMs to 0', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(0); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be greater than 0'); + } + }); + + it('should throw error when setting delayMs to <0', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(-1); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be greater than 0'); + } + }); + + it('should throw error when setting delayMs to a non-integer', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(0.1); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be integer'); + } + }); + + it('should throw error when setting delayMs to a non-safe integer', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(2 ** 53); + } catch (ex) { + expect((ex as Error).message).to.equal('delayMs must be less than or equal to 2^53 - 1'); + } + }); + + it('should throw error when setting base to 0', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(100, false, 0); + } catch (ex) { + expect((ex as Error).message).to.equal('base must be greater than 0'); + } + }); + + it('should throw error when setting base to <0', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(100, false, -1); + } catch (ex) { + expect((ex as Error).message).to.equal('base must be greater than 0'); + } + }); + + it('should throw error when setting base to a non-integer', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(100, false, 0.1); + } catch (ex) { + expect((ex as Error).message).to.equal('base must be integer'); + } + }); + + it('should throw error when setting base to a non-safe integer', (): void => { + try { + BackoffStrategyFactory.exponentialBackoff(100, false, 2 ** 53); + } catch (ex) { + expect((ex as Error).message).to.equal('base must be less than or equal to 2^53 - 1'); + } + }); }); describe('jitteredBackoff', (): void => { it('should produce a jittered backoff strategy', async (): Promise => { const entropyProvider = new NodeJsEntropyProvider(); const randomGenerator = new RandomGenerator(entropyProvider); - const strategy = BackoffStrategyFactory.jitteredBackoff(0, 100, false, randomGenerator); + const strategy = BackoffStrategyFactory.jitteredBackoff(1, 100, false, randomGenerator); expect(await strategy(1)) .to.be.at.least(0) @@ -115,7 +243,7 @@ describe('BackoffStrategyFactory', (): void => { > => { const entropyProvider = new NodeJsEntropyProvider(); const randomGenerator = new RandomGenerator(entropyProvider); - const strategy = BackoffStrategyFactory.jitteredBackoff(0, 100, true, randomGenerator); + const strategy = BackoffStrategyFactory.jitteredBackoff(1, 100, true, randomGenerator); expect(await strategy(1)).to.equal(0); @@ -143,7 +271,7 @@ describe('BackoffStrategyFactory', (): void => { // @ts-ignore global.window = windowMock(); - const strategy = BackoffStrategyFactory.jitteredBackoff(0, 100); + const strategy = BackoffStrategyFactory.jitteredBackoff(1, 100); expect(await strategy(1)) .to.be.at.least(0) @@ -169,5 +297,115 @@ describe('BackoffStrategyFactory', (): void => { // @ts-ignore global.window = undefined; }); + + it('should throw error when setting minDelayMs to 0', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(0, 100, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('minDelayMs must be greater than 0'); + } + }); + + it('should throw error when setting minDelayMs to <0', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(-1, 100, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('minDelayMs must be greater than 0'); + } + }); + + it('should throw error when setting minDelayMs to a non-integer', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(0.1, 100, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('minDelayMs must be integer'); + } + }); + + it('should throw error when setting minDelayMs to a non-safe integer', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(2 ** 53, 100, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('minDelayMs must be less than or equal to 2^53 - 1'); + } + }); + + it('should throw error when setting maxDelayMs to 0', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(1, 0, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('maxDelayMs must be greater than 0'); + } + }); + + it('should throw error when setting maxDelayMs to <0', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(1, -1, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('maxDelayMs must be greater than 0'); + } + }); + + it('should throw error when setting maxDelayMs to a non-integer', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(1, 0.1, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('maxDelayMs must be integer'); + } + }); + + it('should throw error when setting maxDelayMs to a non-safe integer', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(1, 2 ** 53, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('maxDelayMs must be less than or equal to 2^53 - 1'); + } + }); + + it('should throw error when setting minDelayMs = maxDelayMs', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(1, 1, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('maxDelayMs must be greater than minDelayMs'); + } + }); + + it('should throw error when setting minDelayMs > maxDelayMs', (): void => { + const entropyProvider = new NodeJsEntropyProvider(); + const randomGenerator = new RandomGenerator(entropyProvider); + + try { + BackoffStrategyFactory.jitteredBackoff(2, 1, false, randomGenerator); + } catch (ex) { + expect((ex as Error).message).to.equal('maxDelayMs must be greater than minDelayMs'); + } + }); }); }); diff --git a/test/specs/timeoutPolicy.test.ts b/test/specs/timeoutPolicy.test.ts index 115b542..5259d86 100644 --- a/test/specs/timeoutPolicy.test.ts +++ b/test/specs/timeoutPolicy.test.ts @@ -243,6 +243,17 @@ describe('TimeoutPolicy', (): void => { } }); + it('should throw error when setting timeoutAfter to 0', (): void => { + const policy = new TimeoutPolicy(); + + try { + policy.timeoutAfter(0); + expect.fail('did not throw'); + } catch (ex) { + expect((ex as Error).message).to.equal('timeoutMs must be greater than 0'); + } + }); + it('should throw error when setting timeoutAfter to <0', (): void => { const policy = new TimeoutPolicy();