Added an optional timeout parameter to AdapterCacheRedis (#20131)

ref
https://linear.app/tryghost/issue/ENG-902/add-an-optional-timeout-in-the-redis-cache-adapter-in-case-redis

- Added an optional timeout parameter to AdapterCacheRedis, so that the
`get(key)` method will return `null` after the timeout if it hasn't
received a response from Redis
- When load testing the `LinkRedirectRepository` with the Redis cache
enabled on staging, we noticed that for some reason Redis stopped
responding to commands for ~30 seconds.
- The `LinkRedirectRepository` was waiting for the Redis cache to
respond and resulted in a drastic increase in response times for link
redirects
- This change will allow us to set a timeout on the `get(key)` method,
so that if Redis doesn't respond within the timeout, the method will
return `null` as if it were a cache miss.
- Then the `LinkRedirectRepository` will fall back to the database and
return the link redirect from the database instead of waiting
indefinitely for Redis to respond
This commit is contained in:
Chris Raible 2024-05-02 20:39:23 -07:00 committed by GitHub
parent b9f7ea65e9
commit d8b629c713
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 53 additions and 4 deletions

View File

@ -17,6 +17,7 @@ class AdapterCacheRedis extends BaseCacheAdapter {
* @param {Object} [config.clusterConfig] - redis cluster config used in case no cache instance provided * @param {Object} [config.clusterConfig] - redis cluster config used in case no cache instance provided
* @param {Object} [config.storeConfig] - extra redis client config used in case no cache instance provided * @param {Object} [config.storeConfig] - extra redis client config used in case no cache instance provided
* @param {Number} [config.ttl] - default cached value Time To Live (expiration) in *seconds* * @param {Number} [config.ttl] - default cached value Time To Live (expiration) in *seconds*
* @param {Number} [config.getTimeoutMilliseconds] - default timeout for cache get operations in *milliseconds*
* @param {Number} [config.refreshAheadFactor] - 0-1 number to use to determine how old (as a percentage of ttl) an entry should be before refreshing it * @param {Number} [config.refreshAheadFactor] - 0-1 number to use to determine how old (as a percentage of ttl) an entry should be before refreshing it
* @param {String} [config.keyPrefix] - prefix to use when building a unique cache key, e.g.: 'some_id:image-sizes:' * @param {String} [config.keyPrefix] - prefix to use when building a unique cache key, e.g.: 'some_id:image-sizes:'
* @param {Boolean} [config.reuseConnection] - specifies if the redis store/connection should be reused within the process * @param {Boolean} [config.reuseConnection] - specifies if the redis store/connection should be reused within the process
@ -60,6 +61,7 @@ class AdapterCacheRedis extends BaseCacheAdapter {
this.ttl = config.ttl; this.ttl = config.ttl;
this.refreshAheadFactor = config.refreshAheadFactor || 0; this.refreshAheadFactor = config.refreshAheadFactor || 0;
this.getTimeoutMilliseconds = config.getTimeoutMilliseconds || null;
this.currentlyExecutingBackgroundRefreshes = new Set(); this.currentlyExecutingBackgroundRefreshes = new Set();
this.keyPrefix = config.keyPrefix; this.keyPrefix = config.keyPrefix;
this._keysPattern = config.keyPrefix ? `${config.keyPrefix}*` : ''; this._keysPattern = config.keyPrefix ? `${config.keyPrefix}*` : '';
@ -160,6 +162,30 @@ class AdapterCacheRedis extends BaseCacheAdapter {
} }
} }
/**
* Returns the specified key from the cache if it exists, otherwise returns null
* - If getTimeoutMilliseconds is set, the method will return a promise that resolves with the value or null if the timeout is exceeded
*
* @param {string} key
*/
async _get(key) {
if (typeof this.getTimeoutMilliseconds !== 'number') {
return this.cache.get(key);
} else {
return new Promise((resolve) => {
const timer = setTimeout(() => {
debug('get', key, 'timeout');
resolve(null);
}, this.getTimeoutMilliseconds);
this.cache.get(key).then((result) => {
clearTimeout(timer);
resolve(result);
});
});
}
}
/** /**
* *
* @param {string} key * @param {string} key
@ -168,7 +194,7 @@ class AdapterCacheRedis extends BaseCacheAdapter {
async get(key, fetchData) { async get(key, fetchData) {
const internalKey = this._buildKey(key); const internalKey = this._buildKey(key);
try { try {
const result = await this.cache.get(internalKey); const result = await this._get(internalKey);
debug(`get ${internalKey}: Cache ${result ? 'HIT' : 'MISS'}`); debug(`get ${internalKey}: Cache ${result ? 'HIT' : 'MISS'}`);
if (!fetchData) { if (!fetchData) {
return result; return result;

View File

@ -59,6 +59,31 @@ describe('Adapter Cache Redis', function () {
assert.equal(value, 'value from cache'); assert.equal(value, 'value from cache');
}); });
it('returns null if getTimeoutMilliseconds is exceeded', async function () {
const redisCacheInstanceStub = {
get: sinon.stub().callsFake(async () => {
return new Promise((resolve) => {
setTimeout(() => {
resolve('value from cache');
}, 200);
});
}),
store: {
getClient: sinon.stub().returns({
on: sinon.stub()
})
}
};
const cache = new RedisCache({
cache: redisCacheInstanceStub,
getTimeoutMilliseconds: 100
});
const value = await cache.get('key');
assert.equal(value, null);
});
it('can update the cache in the case of a cache miss', async function () { it('can update the cache in the case of a cache miss', async function () {
const KEY = 'update-cache-on-miss'; const KEY = 'update-cache-on-miss';
let cachedValue = null; let cachedValue = null;

View File

@ -159,9 +159,9 @@ module.exports = class LinkRedirectRepository {
if (this.#cache) { if (this.#cache) {
const cachedLink = await this.#cache.get(from); const cachedLink = await this.#cache.get(from);
debug(`getByUrl ${url}: Cache ${cachedLink ? 'HIT' : 'MISS'}`);
// Cache hit, serve from cache // Cache hit, serve from cache
if (cachedLink) { if (cachedLink) {
debug('Cache hit for', from);
return this.#fromSerialized(cachedLink); return this.#fromSerialized(cachedLink);
} }
} }
@ -174,8 +174,6 @@ module.exports = class LinkRedirectRepository {
if (linkRedirectModel) { if (linkRedirectModel) {
const linkRedirect = this.fromModel(linkRedirectModel); const linkRedirect = this.fromModel(linkRedirectModel);
if (this.#cache) { if (this.#cache) {
debug('Cache miss for', from, '. Caching');
// Cache the link
this.#cache.set(from, this.#serialize(linkRedirect)); this.#cache.set(from, this.#serialize(linkRedirect));
} }
return linkRedirect; return linkRedirect;