From 0a9c3470b254a29d64a45a8c8ea6c3f42d329abb Mon Sep 17 00:00:00 2001 From: Ilya Amelevich Date: Fri, 31 Mar 2023 17:11:31 +0200 Subject: [PATCH 1/4] feat: add cacheTTL option Signed-off-by: Ilya Amelevich --- lib/circuit.js | 57 +++++++++++++++++++++++++++++++++++++------------- test/test.js | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 14 deletions(-) diff --git a/lib/circuit.js b/lib/circuit.js index 8cf4da90..f67c0e93 100644 --- a/lib/circuit.js +++ b/lib/circuit.js @@ -86,6 +86,8 @@ Please use options.errorThresholdPercentage`; * has been cached that value will be returned for every subsequent execution: * the cache can be cleared using `clearCache`. (The metrics `cacheHit` and * `cacheMiss` reflect cache activity.) Default: false + * @param {Number} options.cacheTTL the time to live for the cache + * in milliseconds. Set 0 for infinity cache. Default: 0 (no TTL) * @param {AbortController} options.abortController this allows Opossum to * signal upon timeout and properly abort your on going requests instead of * leaving it in the background @@ -146,6 +148,7 @@ class CircuitBreaker extends EventEmitter { ? options.capacity : Number.MAX_SAFE_INTEGER; this.options.errorFilter = options.errorFilter || (_ => false); + this.options.cacheTTL = options.cacheTTL ?? 0; this.semaphore = new Semaphore(this.options.capacity); @@ -275,9 +278,6 @@ class CircuitBreaker extends EventEmitter { this.close(); } }); - if (this.options.cache) { - CACHE.set(this, undefined); - } // Prepopulate the State of the Breaker if (this[SHUTDOWN]) { @@ -557,6 +557,9 @@ class CircuitBreaker extends EventEmitter { } const args = Array.prototype.slice.call(rest); + const cache = this.getCache(); + const cacheKey = JSON.stringify(rest); + /** * Emitted when the circuit breaker action is executed * @event CircuitBreaker#fire @@ -564,15 +567,20 @@ class CircuitBreaker extends EventEmitter { */ this.emit('fire', args); - if (CACHE.get(this) !== undefined) { - /** - * Emitted when the circuit breaker is using the cache - * and finds a value. - * @event CircuitBreaker#cacheHit - */ - this.emit('cacheHit'); - return CACHE.get(this); - } else if (this.options.cache) { + // If cache is enabled, check if we have a cached value + if (this.options.cache) { + const cached = cache.get(cacheKey); + if (cached && + (cached.expiresAt === 0 || cached.expiresAt >= Date.now()) + ) { + /** + * Emitted when the circuit breaker is using the cache + * and finds a value. + * @event CircuitBreaker#cacheHit + */ + this.emit('cacheHit'); + return cached.value; + } /** * Emitted when the circuit breaker does not find a value in * the cache, but the cache option is enabled. @@ -649,7 +657,12 @@ class CircuitBreaker extends EventEmitter { this.semaphore.release(); resolve(result); if (this.options.cache) { - CACHE.set(this, promise); + cache.set(cacheKey, { + expiresAt: this.options.cacheTTL > 0 + ? Date.now() + this.options.cacheTTL + : 0, + value: promise + }); } } }) @@ -686,7 +699,23 @@ class CircuitBreaker extends EventEmitter { * @returns {void} */ clearCache () { - CACHE.set(this, undefined); + CACHE.delete(this); + } + + /** + * Return cache of this {@link CircuitBreaker} + * @returns {Map} the cache of this {@link CircuitBreaker} + */ + getCache () { + let cache = CACHE.get(this); + if (!(cache instanceof Map)) { + cache = new Map(); + CACHE.set(this, cache); + } + return cache; } /** diff --git a/test/test.js b/test/test.js index 5509f87f..d2fe8f6d 100644 --- a/test/test.js +++ b/test/test.js @@ -113,6 +113,48 @@ test('Using cache', t => { }); }); +test('Using cache with TTL', t => { + t.plan(12); + const expected = 34; + const options = { + cache: true, + cacheTTL: 100 + }; + const breaker = new CircuitBreaker(passFail, options); + + return breaker.fire(expected) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 0, 'does not hit the cache'); + t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); + t.equals(stats.fires, 1, 'fired once'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 1, 'did not emit miss'); + t.equals(stats.fires, 2, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + // wait 100ms for the cache to expire + .then(() => new Promise(resolve => setTimeout(resolve, 100))) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 2, 'did not emit miss'); + t.equals(stats.fires, 3, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(t.end) + .catch(t.fail); +}); + test('Fails when the circuit function fails', t => { t.plan(2); const breaker = new CircuitBreaker(passFail); From bea30947024cffc834975c2301ad938d8a4d4656 Mon Sep 17 00:00:00 2001 From: Ilya Amelevich Date: Mon, 1 May 2023 18:55:06 +0200 Subject: [PATCH 2/4] feat: add `cacheGetKey` and `cacheTransport` options refactor: move default cache implementation to the separate file Signed-off-by: Ilya Amelevich --- lib/cache.js | 50 ++++++++++++++++++++++++++++++++++++ lib/circuit.js | 69 +++++++++++++++++++++++++++++--------------------- 2 files changed, 90 insertions(+), 29 deletions(-) create mode 100644 lib/cache.js diff --git a/lib/cache.js b/lib/cache.js new file mode 100644 index 00000000..b48a2733 --- /dev/null +++ b/lib/cache.js @@ -0,0 +1,50 @@ +/** + * Simple in-memory cache implementation + * @class MemoryCache + * @property {Map} cache Cache map + */ +class MemoryCache { + constructor () { + this.cache = new Map(); + } + + /** + * Get cache value by key + * @param {string} key Cache key + * @return {any} Response from cache + */ + get (key) { + const cached = this.cache.get(key); + if (cached) { + if (cached.expiresAt > Date.now() || cached.expiresAt === 0) { + return cached.value; + } + this.cache.delete(key); + } + return undefined; + } + + /** + * Set cache key with value and ttl + * @param {string} key Cache key + * @param {any} value Value to cache + * @param {number} ttl Time to live in milliseconds + * @return {void} + */ + set (key, value, ttl) { + this.cache.set(key, { + expiresAt: ttl, + value + }); + } + + /** + * Clear cache + * @returns {void} + */ + flush () { + this.cache.clear(); + } +} + +module.exports = exports = MemoryCache; diff --git a/lib/circuit.js b/lib/circuit.js index f67c0e93..0911d6b5 100644 --- a/lib/circuit.js +++ b/lib/circuit.js @@ -3,6 +3,7 @@ const EventEmitter = require('events'); const Status = require('./status'); const Semaphore = require('./semaphore'); +const MemoryCache = require('./cache'); const STATE = Symbol('state'); const OPEN = Symbol('open'); @@ -14,7 +15,6 @@ const FALLBACK_FUNCTION = Symbol('fallback'); const STATUS = Symbol('status'); const NAME = Symbol('name'); const GROUP = Symbol('group'); -const CACHE = new WeakMap(); const ENABLED = Symbol('Enabled'); const WARMING_UP = Symbol('warming-up'); const VOLUME_THRESHOLD = Symbol('volume-threshold'); @@ -88,6 +88,13 @@ Please use options.errorThresholdPercentage`; * `cacheMiss` reflect cache activity.) Default: false * @param {Number} options.cacheTTL the time to live for the cache * in milliseconds. Set 0 for infinity cache. Default: 0 (no TTL) + * @param {Function} options.cacheGetKey function that returns the key to use + * when caching the result of the circuit's fire. + * Better to use custom one, because `JSON.stringify` is not good + * from performance perspective. + * Default: `(...args) => JSON.stringify(args)` + * @param {CacheTransport} options.cacheTransport custom cache transport + * should implement `get`, `set` and `flush` methods. * @param {AbortController} options.abortController this allows Opossum to * signal upon timeout and properly abort your on going requests instead of * leaving it in the background @@ -149,6 +156,23 @@ class CircuitBreaker extends EventEmitter { : Number.MAX_SAFE_INTEGER; this.options.errorFilter = options.errorFilter || (_ => false); this.options.cacheTTL = options.cacheTTL ?? 0; + this.options.cacheGetKey = options.cacheGetKey ?? + ((...args) => JSON.stringify(args)); + + // Set default cache transport if not provided + if (this.options.cache) { + if (this.options.cacheTransport === undefined) { + this.options.cacheTransport = new MemoryCache(); + } else if (typeof this.options.cacheTransport !== 'object' || + !this.options.cacheTransport.get || + !this.options.cacheTransport.set || + !this.options.cacheTransport.flush + ) { + throw new TypeError( + 'options.cacheTransport should be an object with `get`, `set` and `flush` methods' + ); + } + } this.semaphore = new Semaphore(this.options.capacity); @@ -365,6 +389,9 @@ class CircuitBreaker extends EventEmitter { } this.status.shutdown(); this[STATE] = SHUTDOWN; + + // clear cache on shutdown + this.clearCache(); } /** @@ -557,8 +584,7 @@ class CircuitBreaker extends EventEmitter { } const args = Array.prototype.slice.call(rest); - const cache = this.getCache(); - const cacheKey = JSON.stringify(rest); + const cacheKey = this.options.cacheGetKey.apply(this, rest); /** * Emitted when the circuit breaker action is executed @@ -569,17 +595,15 @@ class CircuitBreaker extends EventEmitter { // If cache is enabled, check if we have a cached value if (this.options.cache) { - const cached = cache.get(cacheKey); - if (cached && - (cached.expiresAt === 0 || cached.expiresAt >= Date.now()) - ) { + const cached = this.options.cacheTransport.get(cacheKey); + if (cached) { /** * Emitted when the circuit breaker is using the cache * and finds a value. * @event CircuitBreaker#cacheHit */ this.emit('cacheHit'); - return cached.value; + return cached; } /** * Emitted when the circuit breaker does not find a value in @@ -657,12 +681,13 @@ class CircuitBreaker extends EventEmitter { this.semaphore.release(); resolve(result); if (this.options.cache) { - cache.set(cacheKey, { - expiresAt: this.options.cacheTTL > 0 + this.options.cacheTransport.set( + cacheKey, + promise, + this.options.cacheTTL > 0 ? Date.now() + this.options.cacheTTL - : 0, - value: promise - }); + : 0 + ); } } }) @@ -699,23 +724,9 @@ class CircuitBreaker extends EventEmitter { * @returns {void} */ clearCache () { - CACHE.delete(this); - } - - /** - * Return cache of this {@link CircuitBreaker} - * @returns {Map} the cache of this {@link CircuitBreaker} - */ - getCache () { - let cache = CACHE.get(this); - if (!(cache instanceof Map)) { - cache = new Map(); - CACHE.set(this, cache); + if (this.options.cache) { + this.options.cacheTransport.flush(); } - return cache; } /** From 30b58046bac695cccf4836751e52b243d8c75e2d Mon Sep 17 00:00:00 2001 From: Ilya Amelevich Date: Mon, 1 May 2023 18:55:47 +0200 Subject: [PATCH 3/4] test: cache tests Signed-off-by: Ilya Amelevich --- test/cache.js | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++ test/test.js | 86 ------------------------- 2 files changed, 170 insertions(+), 86 deletions(-) create mode 100644 test/cache.js diff --git a/test/cache.js b/test/cache.js new file mode 100644 index 00000000..74b9020e --- /dev/null +++ b/test/cache.js @@ -0,0 +1,170 @@ +'use strict'; + +const test = require('tape'); +const CircuitBreaker = require('../'); +const common = require('./common'); + +const passFail = common.passFail; + +test('Using cache', t => { + t.plan(9); + const expected = 34; + const options = { + cache: true + }; + const breaker = new CircuitBreaker(passFail, options); + + breaker.fire(expected) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 0, 'does not hit the cache'); + t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); + t.equals(stats.fires, 1, 'fired once'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 1, 'did not emit miss'); + t.equals(stats.fires, 2, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + breaker.clearCache(); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.shutdown()) + .then(t.end) + .catch(t.fail); +}); + +test('Using cache with TTL', t => { + t.plan(12); + const expected = 34; + const options = { + cache: true, + cacheTTL: 100 + }; + const breaker = new CircuitBreaker(passFail, options); + + return breaker.fire(expected) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 0, 'does not hit the cache'); + t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); + t.equals(stats.fires, 1, 'fired once'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 1, 'did not emit miss'); + t.equals(stats.fires, 2, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + // wait 100ms for the cache to expire + .then(() => new Promise(resolve => setTimeout(resolve, 100))) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 2, 'did not emit miss'); + t.equals(stats.fires, 3, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(t.end) + .catch(t.fail); +}); + +test('Using cache with custom get cache key', t => { + t.plan(9); + const expected = 34; + const options = { + cache: true, + cacheGetKey: x => `key-${x}` + }; + const breaker = new CircuitBreaker(passFail, options); + + breaker.fire(expected) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 0, 'does not hit the cache'); + t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); + t.equals(stats.fires, 1, 'fired once'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 1, 'did not emit miss'); + t.equals(stats.fires, 2, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + breaker.clearCache(); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.shutdown()) + .then(t.end) + .catch(t.fail); +}); + +test('Using cache with custom transport', t => { + t.plan(9); + const expected = 34; + const cache = new Map(); + const options = { + cache: true, + cacheTransport: { + get: key => cache.get(key), + set: (key, value) => cache.set(key, value), + flush: () => cache.clear() + } + }; + const breaker = new CircuitBreaker(passFail, options); + + breaker.fire(expected) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 0, 'does not hit the cache'); + t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); + t.equals(stats.fires, 1, 'fired once'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(stats.cacheHits, 1, 'hit the cache'); + t.equals(stats.cacheMisses, 1, 'did not emit miss'); + t.equals(stats.fires, 2, 'fired twice'); + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + breaker.clearCache(); + }) + .then(() => breaker.fire(expected)) + .then(arg => { + const stats = breaker.status.stats; + t.equals(arg, expected, + `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); + }) + .then(() => breaker.shutdown()) + .then(t.end) + .catch(t.fail); +}); diff --git a/test/test.js b/test/test.js index d2fe8f6d..3aecc8a2 100644 --- a/test/test.js +++ b/test/test.js @@ -69,92 +69,6 @@ test('Passes parameters to the circuit function', t => { .catch(t.fail); }); -test('Using cache', t => { - t.plan(9); - const expected = 34; - const options = { - cache: true - }; - const breaker = new CircuitBreaker(passFail, options); - - breaker.fire(expected) - .then(arg => { - const stats = breaker.status.stats; - t.equals(stats.cacheHits, 0, 'does not hit the cache'); - t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); - t.equals(stats.fires, 1, 'fired once'); - t.equals(arg, expected, - `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); - }) - .catch(t.fail) - .then(() => { - breaker.fire(expected) - .then(arg => { - const stats = breaker.status.stats; - t.equals(stats.cacheHits, 1, 'hit the cache'); - t.equals(stats.cacheMisses, 1, 'did not emit miss'); - t.equals(stats.fires, 2, 'fired twice'); - t.equals(arg, expected, - `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); - breaker.clearCache(); - }) - .catch(t.fail) - .then(() => { - breaker.fire(expected) - .then(arg => { - const stats = breaker.status.stats; - t.equals(arg, expected, - `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); - }) - // .then(_ => breaker.shutdown()) - .then(t.end) - .catch(t.fail); - }); - }); -}); - -test('Using cache with TTL', t => { - t.plan(12); - const expected = 34; - const options = { - cache: true, - cacheTTL: 100 - }; - const breaker = new CircuitBreaker(passFail, options); - - return breaker.fire(expected) - .then(arg => { - const stats = breaker.status.stats; - t.equals(stats.cacheHits, 0, 'does not hit the cache'); - t.equals(stats.cacheMisses, 1, 'emits a cacheMiss'); - t.equals(stats.fires, 1, 'fired once'); - t.equals(arg, expected, - `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); - }) - .then(() => breaker.fire(expected)) - .then(arg => { - const stats = breaker.status.stats; - t.equals(stats.cacheHits, 1, 'hit the cache'); - t.equals(stats.cacheMisses, 1, 'did not emit miss'); - t.equals(stats.fires, 2, 'fired twice'); - t.equals(arg, expected, - `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); - }) - // wait 100ms for the cache to expire - .then(() => new Promise(resolve => setTimeout(resolve, 100))) - .then(() => breaker.fire(expected)) - .then(arg => { - const stats = breaker.status.stats; - t.equals(stats.cacheHits, 1, 'hit the cache'); - t.equals(stats.cacheMisses, 2, 'did not emit miss'); - t.equals(stats.fires, 3, 'fired twice'); - t.equals(arg, expected, - `cache hits:misses ${stats.cacheHits}:${stats.cacheMisses}`); - }) - .then(t.end) - .catch(t.fail); -}); - test('Fails when the circuit function fails', t => { t.plan(2); const breaker = new CircuitBreaker(passFail); From c2e54b8f4b6b9e3fddf22d0ce46b610884151eff Mon Sep 17 00:00:00 2001 From: Ilya Amelevich Date: Wed, 24 May 2023 15:07:24 +0200 Subject: [PATCH 4/4] fix: calculate `cacheKey` only if cache is enabled Signed-off-by: Ilya Amelevich --- lib/circuit.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/circuit.js b/lib/circuit.js index 0911d6b5..a36f9278 100644 --- a/lib/circuit.js +++ b/lib/circuit.js @@ -584,7 +584,8 @@ class CircuitBreaker extends EventEmitter { } const args = Array.prototype.slice.call(rest); - const cacheKey = this.options.cacheGetKey.apply(this, rest); + // Need to create variable here to prevent extra calls if cache is disabled + let cacheKey = ''; /** * Emitted when the circuit breaker action is executed @@ -595,6 +596,7 @@ class CircuitBreaker extends EventEmitter { // If cache is enabled, check if we have a cached value if (this.options.cache) { + cacheKey = this.options.cacheGetKey.apply(this, rest); const cached = this.options.cacheTransport.get(cacheKey); if (cached) { /**