Skip to content

Commit

Permalink
[TECH] Changer le system de Redis lock et supprimer Bluebird.
Browse files Browse the repository at this point in the history 
 #10099
  • Loading branch information
@pix-service-auto-merge
pix-service-auto-merge authored Sep 16, 2024
2 parents 78dd1bd + 94b6344 commit 2ffea5c
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 84 deletions.
1 change: 0 additions & 1 deletion api/package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion api/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
"@xmldom/xmldom": "^0.7.5",
"axios": "^1.0.0",
"bcrypt": "^5.0.1",
"bluebird": "^3.7.2",
"bookshelf": "^1.2.0",
"bookshelf-validate": "^2.0.3",
"cron-parser": "^4.9.0",
Expand Down
19 changes: 7 additions & 12 deletions api/src/shared/infrastructure/caches/RedisCache.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
import bluebird from 'bluebird';

const { using } = bluebird;

import Redlock from 'redlock';

import { config } from '../../config.js';
Expand Down Expand Up @@ -38,17 +34,14 @@ class RedisCache extends Cache {

async _manageValueNotFoundInCache(key, generator) {
const keyToLock = REDIS_LOCK_PREFIX + key;
const retrieveAndSetValue = async () => {

let lock;
try {
lock = await this._client.lock(keyToLock, config.caching.redisCacheKeyLockTTL);

logger.info({ key }, 'Executing generator for Redis key');
const value = await generator();
return this.set(key, value);
};
const unlockErrorHandler = (err) => logger.error({ key }, 'Error while trying to unlock Redis key', err);

try {
const locker = this._client.lockDisposer(keyToLock, config.caching.redisCacheKeyLockTTL, unlockErrorHandler);
const value = await using(locker, retrieveAndSetValue);
return value;
} catch (err) {
if (err instanceof Redlock.LockError) {
logger.trace({ keyToLock }, 'Could not lock Redis key, waiting');
Expand All @@ -57,6 +50,8 @@ class RedisCache extends Cache {
}
logger.error({ err }, 'Error while trying to update value in Redis cache');
throw err;
} finally {
if (lock) await lock.unlock();
}
}

Expand Down
2 changes: 1 addition & 1 deletion api/src/shared/infrastructure/utils/RedisClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class RedisClient {
this.keys = this._wrapWithPrefix(this._client.keys).bind(this._client);
this.ping = this._client.ping.bind(this._client);
this.flushall = this._client.flushall.bind(this._client);
this.lockDisposer = this._clientWithLock.disposer.bind(this._clientWithLock);
this.lock = this._clientWithLock.lock.bind(this._clientWithLock);
}

_wrapWithPrefix(fn) {
Expand Down
50 changes: 38 additions & 12 deletions api/tests/shared/integration/infrastructure/utils/RedisClient_test.js
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import { randomUUID } from 'node:crypto';

import bluebird from 'bluebird';
import Redis from 'ioredis';
import Redlock from 'redlock';

import { config } from '../../../../../src/shared/config.js';
import { RedisClient } from '../../../../../src/shared/infrastructure/utils/RedisClient.js';
import { expect } from '../../../../test-helper.js';

const { using } = bluebird;
import { catchErr, expect } from '../../../../test-helper.js';

describe('Integration | Infrastructure | Utils | RedisClient', function () {
beforeEach(async function () {
const redisClient = new RedisClient(config.redis.url);
await redisClient.flushall();
});

it('stores and retrieve a value for a key', async function () {
// given
const key = new Date().toISOString();
Expand Down Expand Up @@ -105,21 +108,44 @@ describe('Integration | Infrastructure | Utils | RedisClient', function () {
expect(result).to.equal('PONG');
});

describe('lock disposer', function () {
it('should provide a lock disposer that grants a lock', async function () {
describe('lock', function () {
it('locks a key', async function () {
// given
const client = new RedisClient(config.redis.url);
const otherClient = new Redis(config.redis.url);

// when
await client.lock('locks:toto', 10000);

// then
const count = await otherClient.exists('locks:toto');
expect(count).to.equal(1);
});

it('unlocks a key', async function () {
// given
const client = new RedisClient(config.redis.url);
const clientWithAllFunctions = new Redis(config.redis.url);
const otherClient = new Redis(config.redis.url);
const lock = await client.lock('locks:toto', 10000);

// when
await lock.unlock();

// then
const count = await otherClient.exists('locks:toto');
expect(count).to.equal(0);
});

const locker = client.lockDisposer('locks:toto', 1000);
it('throws a LockError when locking a locked resource', async function () {
// given
const client = new RedisClient(config.redis.url);
await client.lock('locks:toto', 10000);

// when
const result = await using(locker, async () => {
return clientWithAllFunctions.exists('locks:toto');
});
const error = await catchErr(client.lock)('locks:toto', 10000);

// then
expect(result).to.equal(1);
expect(error).to.be.instanceOf(Redlock.LockError);
});
});

Expand Down
91 changes: 34 additions & 57 deletions api/tests/shared/unit/infrastructure/caches/RedisCache_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {

beforeEach(function () {
stubbedClient = {
lockDisposer: sinon.stub().resolves(() => {
return;
}),
lock: sinon.stub().resolves({ unlock: sinon.stub().resolves() }),
};
sinon.stub(RedisCache, 'createClient').withArgs(REDIS_URL).returns(stubbedClient);
redisCache = new RedisCache(REDIS_URL);
Expand All @@ -30,22 +28,21 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {
});

context('when the value is already in cache', function () {
it('should resolve with the existing value', function () {
it('should resolve with the existing value', async function () {
// given
const cachedData = { foo: 'bar' };
const redisCachedData = JSON.stringify(cachedData);
stubbedClient.get.withArgs(CACHE_KEY).resolves(redisCachedData);
stubbedClient.lrange.withArgs(`${CACHE_KEY}:${PATCHES_KEY}`, 0, -1).resolves([]);

// when
const promise = redisCache.get(CACHE_KEY);
const result = await redisCache.get(CACHE_KEY);

// then
return expect(promise).to.have.been.fulfilled.then((result) => {
expect(result).to.deep.equal(cachedData);
});
expect(result).to.deep.equal(cachedData);
});

it('should resolve with the existing value and apply the patche if any', function () {
it('should resolve with the existing value and apply the patche if any', async function () {
// given
const redisCachedData = JSON.stringify({ foo: 'bar' });
const cachedPatchesData = [JSON.stringify({ operation: 'assign', path: 'foo', value: 'roger' })];
Expand All @@ -54,15 +51,13 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {
const finalResult = { foo: 'roger' };

// when
const promise = redisCache.get(CACHE_KEY);
const result = await redisCache.get(CACHE_KEY);

// then
return expect(promise).to.have.been.fulfilled.then((result) => {
expect(result).to.deep.equal(finalResult);
});
expect(result).to.deep.equal(finalResult);
});

it('should resolve with the existing value and apply the patches if any', function () {
it('should resolve with the existing value and apply the patches if any', async function () {
// given
const redisCachedData = JSON.stringify({ foo: 'bar', fibonnaci: [1] });
const cachedPatchesData = [
Expand All @@ -76,12 +71,10 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {
const finalResult = { foo: 'roger', fibonnaci: [1, 2, 5] };

// when
const promise = redisCache.get(CACHE_KEY);
const result = await redisCache.get(CACHE_KEY);

// then
return expect(promise).to.have.been.fulfilled.then((result) => {
expect(result).to.deep.equal(finalResult);
});
expect(result).to.deep.equal(finalResult);
});
});

Expand All @@ -94,69 +87,57 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {
redisCache.set.resolves();
});

it('should try to lock the cache key', function () {
it('should try to lock the cache key', async function () {
// given
const expectedLockedKey = 'locks:' + CACHE_KEY;
const handler = sinon.stub().resolves();

// when
const promise = redisCache.get(CACHE_KEY, handler);
await redisCache.get(CACHE_KEY, handler);

// then
return promise.then(() => {
return expect(stubbedClient.lockDisposer).to.have.been.calledWith(
expectedLockedKey,
settings.caching.redisCacheKeyLockTTL,
);
});
expect(stubbedClient.lock).to.have.been.calledWith(expectedLockedKey, settings.caching.redisCacheKeyLockTTL);
});

context('and the cache key is not already locked', function () {
it('should add into the cache the value returned by the handler', function () {
it('should add into the cache the value returned by the handler', async function () {
// given
const dataFromHandler = { name: 'data from learning content' };
const handler = () => Promise.resolve(dataFromHandler);
const handler = sinon.stub().resolves(dataFromHandler);

// when
const promise = redisCache.get(CACHE_KEY, handler);
await redisCache.get(CACHE_KEY, handler);

// then
return promise.then(() => {
return expect(redisCache.set).to.have.been.calledWithExactly(CACHE_KEY, dataFromHandler);
});
expect(redisCache.set).to.have.been.calledWithExactly(CACHE_KEY, dataFromHandler);
});

it('should return the value', function () {
it('should return the value', async function () {
// given
const dataFromHandler = { name: 'data from learning content' };
const handler = () => Promise.resolve(dataFromHandler);
const handler = sinon.stub().resolves(dataFromHandler);
redisCache.set.resolves(dataFromHandler);

// when
const promise = redisCache.get(CACHE_KEY, handler);
const value = await redisCache.get(CACHE_KEY, handler);

// then
return promise.then((value) => {
return expect(value).to.equal(dataFromHandler);
});
expect(value).to.equal(dataFromHandler);
});
});

context('and the cache key is already locked', function () {
it('should wait and retry to get the value from the cache', function () {
it('should wait and retry to get the value from the cache', async function () {
// given
stubbedClient.lockDisposer.rejects(new Redlock.LockError());
const handler = () => {
return;
};
const dataFromHandler = { name: 'data from learning content' };
const handler = sinon.stub().resolves(dataFromHandler);
stubbedClient.lock.rejects(new Redlock.LockError());

// when
const promise = redisCache.get(CACHE_KEY, handler);
await redisCache.get(CACHE_KEY, handler);

// then
return promise.then(() => {
expect(stubbedClient.get).to.have.been.calledTwice;
});
expect(stubbedClient.get).to.have.been.calledTwice;
});
});
});
Expand Down Expand Up @@ -194,18 +175,16 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {
stubbedClient.del = sinon.stub();
});

it('should resolve with the object to cache', function () {
it('should resolve with the object to cache', async function () {
// given
stubbedClient.set.resolves();

// when
const promise = redisCache.set(CACHE_KEY, objectToCache);
const result = await redisCache.set(CACHE_KEY, objectToCache);

// then
return expect(promise).to.have.been.fulfilled.then((result) => {
expect(result).to.deep.equal(objectToCache);
expect(stubbedClient.set).to.have.been.calledWithExactly(CACHE_KEY, JSON.stringify(objectToCache));
});
expect(result).to.deep.equal(objectToCache);
expect(stubbedClient.set).to.have.been.calledWithExactly(CACHE_KEY, JSON.stringify(objectToCache));
});

it('should reject when the Redis cache client throws an error', function () {
Expand All @@ -225,12 +204,10 @@ describe('Unit | Infrastructure | Cache | redis-cache', function () {
stubbedClient.del.resolves();

// when
const promise = redisCache.set(CACHE_KEY, objectToCache);
await redisCache.set(CACHE_KEY, objectToCache);

// then
return expect(promise).to.have.been.fulfilled.then(() => {
expect(stubbedClient.del).to.have.been.calledWithExactly(`${CACHE_KEY}:patches`);
});
expect(stubbedClient.del).to.have.been.calledWithExactly(`${CACHE_KEY}:patches`);
});
});

Expand Down

0 comments on commit 2ffea5c

Please sign in to comment.