From 7be80d1479488753450291eab408ca10869d84a2 Mon Sep 17 00:00:00 2001 From: Nikolay Karadzhov Date: Wed, 14 May 2025 14:55:20 +0300 Subject: [PATCH 1/3] fix(client): cache subsequent clients we dont need to recreate a client if its config hasnt changed fixes #2954 --- packages/client/lib/client/index.ts | 30 +++++--- packages/client/lib/client/pool.ts | 27 ++++--- packages/client/lib/cluster/index.ts | 30 +++++--- .../client/lib/single-entry-cache.spec.ts | 73 +++++++++++++++++++ packages/client/lib/single-entry-cache.ts | 25 +++++++ 5 files changed, 154 insertions(+), 31 deletions(-) create mode 100644 packages/client/lib/single-entry-cache.spec.ts create mode 100644 packages/client/lib/single-entry-cache.ts diff --git a/packages/client/lib/client/index.ts b/packages/client/lib/client/index.ts index f9e95025c6a..e4b781fb5aa 100644 --- a/packages/client/lib/client/index.ts +++ b/packages/client/lib/client/index.ts @@ -17,6 +17,7 @@ import { RedisLegacyClient, RedisLegacyClientType } from './legacy-mode'; import { RedisPoolOptions, RedisClientPool } from './pool'; import { RedisVariadicArgument, parseArgs, pushVariadicArguments } from '../commands/generic-transformers'; import { BasicCommandParser, CommandParser } from './parser'; +import SingleEntryCache from '../single-entry-cache'; export interface RedisClientOptions< M extends RedisModules = RedisModules, @@ -206,23 +207,32 @@ export default class RedisClient< } } + static #SingleEntryCache = new SingleEntryCache() + static factory< M extends RedisModules = {}, F extends RedisFunctions = {}, S extends RedisScripts = {}, RESP extends RespVersions = 2 >(config?: CommanderConfig) { - const Client = attachConfig({ - BaseClass: RedisClient, - commands: COMMANDS, - createCommand: RedisClient.#createCommand, - createModuleCommand: RedisClient.#createModuleCommand, - createFunctionCommand: RedisClient.#createFunctionCommand, - createScriptCommand: RedisClient.#createScriptCommand, - config - }); - Client.prototype.Multi = RedisClientMultiCommand.extend(config); + + let Client = RedisClient.#SingleEntryCache.get(config); + if (!Client) { + Client = attachConfig({ + BaseClass: RedisClient, + commands: COMMANDS, + createCommand: RedisClient.#createCommand, + createModuleCommand: RedisClient.#createModuleCommand, + createFunctionCommand: RedisClient.#createFunctionCommand, + createScriptCommand: RedisClient.#createScriptCommand, + config + }); + + Client.prototype.Multi = RedisClientMultiCommand.extend(config); + + RedisClient.#SingleEntryCache.set(config, Client); + } return ( options?: Omit, keyof Exclude> diff --git a/packages/client/lib/client/pool.ts b/packages/client/lib/client/pool.ts index a08377e3d38..086d6ba192b 100644 --- a/packages/client/lib/client/pool.ts +++ b/packages/client/lib/client/pool.ts @@ -8,6 +8,7 @@ import { attachConfig, functionArgumentsPrefix, getTransformReply, scriptArgumen import { CommandOptions } from './commands-queue'; import RedisClientMultiCommand, { RedisClientMultiCommandType } from './multi-command'; import { BasicCommandParser } from './parser'; +import SingleEntryCache from '../single-entry-cache'; export interface RedisPoolOptions { /** @@ -110,6 +111,8 @@ export class RedisClientPool< }; } + static #SingleEntryCache = new SingleEntryCache(); + static create< M extends RedisModules, F extends RedisFunctions, @@ -120,17 +123,21 @@ export class RedisClientPool< clientOptions?: RedisClientOptions, options?: Partial ) { - const Pool = attachConfig({ - BaseClass: RedisClientPool, - commands: COMMANDS, - createCommand: RedisClientPool.#createCommand, - createModuleCommand: RedisClientPool.#createModuleCommand, - createFunctionCommand: RedisClientPool.#createFunctionCommand, - createScriptCommand: RedisClientPool.#createScriptCommand, - config: clientOptions - }); - Pool.prototype.Multi = RedisClientMultiCommand.extend(clientOptions); + let Pool = RedisClientPool.#SingleEntryCache.get(clientOptions); + if(!Pool) { + Pool = attachConfig({ + BaseClass: RedisClientPool, + commands: COMMANDS, + createCommand: RedisClientPool.#createCommand, + createModuleCommand: RedisClientPool.#createModuleCommand, + createFunctionCommand: RedisClientPool.#createFunctionCommand, + createScriptCommand: RedisClientPool.#createScriptCommand, + config: clientOptions + }); + Pool.prototype.Multi = RedisClientMultiCommand.extend(clientOptions); + RedisClientPool.#SingleEntryCache.set(clientOptions, Pool); + } // returning a "proxy" to prevent the namespaces._self to leak between "proxies" return Object.create( diff --git a/packages/client/lib/cluster/index.ts b/packages/client/lib/cluster/index.ts index 622ac78f467..8ae80c8468f 100644 --- a/packages/client/lib/cluster/index.ts +++ b/packages/client/lib/cluster/index.ts @@ -12,6 +12,7 @@ import { RedisTcpSocketOptions } from '../client/socket'; import ASKING from '../commands/ASKING'; import { BasicCommandParser } from '../client/parser'; import { parseArgs } from '../commands/generic-transformers'; +import SingleEntryCache from '../single-entry-cache'; interface ClusterCommander< M extends RedisModules, @@ -213,6 +214,8 @@ export default class RedisCluster< }; } + static #SingleEntryCache = new SingleEntryCache(); + static factory< M extends RedisModules = {}, F extends RedisFunctions = {}, @@ -221,17 +224,22 @@ export default class RedisCluster< TYPE_MAPPING extends TypeMapping = {}, // POLICIES extends CommandPolicies = {} >(config?: ClusterCommander) { - const Cluster = attachConfig({ - BaseClass: RedisCluster, - commands: COMMANDS, - createCommand: RedisCluster.#createCommand, - createModuleCommand: RedisCluster.#createModuleCommand, - createFunctionCommand: RedisCluster.#createFunctionCommand, - createScriptCommand: RedisCluster.#createScriptCommand, - config - }); - - Cluster.prototype.Multi = RedisClusterMultiCommand.extend(config); + + let Cluster = RedisCluster.#SingleEntryCache.get(config); + if (!Cluster) { + Cluster = attachConfig({ + BaseClass: RedisCluster, + commands: COMMANDS, + createCommand: RedisCluster.#createCommand, + createModuleCommand: RedisCluster.#createModuleCommand, + createFunctionCommand: RedisCluster.#createFunctionCommand, + createScriptCommand: RedisCluster.#createScriptCommand, + config + }); + + Cluster.prototype.Multi = RedisClusterMultiCommand.extend(config); + RedisCluster.#SingleEntryCache.set(config, Cluster); + } return (options?: Omit>) => { // returning a "proxy" to prevent the namespaces._self to leak between "proxies" diff --git a/packages/client/lib/single-entry-cache.spec.ts b/packages/client/lib/single-entry-cache.spec.ts new file mode 100644 index 00000000000..ed6ea206a60 --- /dev/null +++ b/packages/client/lib/single-entry-cache.spec.ts @@ -0,0 +1,73 @@ +import assert from 'node:assert'; +import SingleEntryCache from './single-entry-cache'; + +describe('SingleEntryCache', () => { + let cache: SingleEntryCache; + beforeEach(() => { + cache = new SingleEntryCache(); + }); + + it('should return undefined when getting from empty cache', () => { + assert.strictEqual(cache.get({ key: 'value' }), undefined); + }); + + it('should return the cached instance when getting with the same key object', () => { + const keyObj = { key: 'value' }; + const instance = { data: 'test data' }; + + cache.set(keyObj, instance); + assert.strictEqual(cache.get(keyObj), instance); + }); + + it('should return undefined when getting with a different key object', () => { + const keyObj1 = { key: 'value1' }; + const keyObj2 = { key: 'value2' }; + const instance = { data: 'test data' }; + + cache.set(keyObj1, instance); + assert.strictEqual(cache.get(keyObj2), undefined); + }); + + it('should update the cached instance when setting with the same key object', () => { + const keyObj = { key: 'value' }; + const instance1 = { data: 'test data 1' }; + const instance2 = { data: 'test data 2' }; + + cache.set(keyObj, instance1); + assert.strictEqual(cache.get(keyObj), instance1); + + cache.set(keyObj, instance2); + assert.strictEqual(cache.get(keyObj), instance2); + }); + + it('should handle undefined key object', () => { + const instance = { data: 'test data' }; + + cache.set(undefined, instance); + assert.strictEqual(cache.get(undefined), instance); + }); + + it('should handle complex objects as keys', () => { + const keyObj = { + id: 123, + nested: { + prop: 'value', + array: [1, 2, 3] + } + }; + const instance = { data: 'complex test data' }; + + cache.set(keyObj, instance); + assert.strictEqual(cache.get(keyObj), instance); + }); + + it('should consider objects with same properties but different order as different keys', () => { + const keyObj1 = { a: 1, b: 2 }; + const keyObj2 = { b: 2, a: 1 }; // Same properties but different order + const instance = { data: 'test data' }; + + cache.set(keyObj1, instance); + + assert.strictEqual(cache.get(keyObj2), undefined); + }); +}); diff --git a/packages/client/lib/single-entry-cache.ts b/packages/client/lib/single-entry-cache.ts new file mode 100644 index 00000000000..c9c6a993466 --- /dev/null +++ b/packages/client/lib/single-entry-cache.ts @@ -0,0 +1,25 @@ +export default class SingleEntryCache { + #cached?: any; + #key?: string; + + /** + * Retrieves an instance from the cache based on the provided key object. + * + * @param keyObj - The key object to look up in the cache. + * @returns The cached instance if found, undefined otherwise. + * + * @remarks + * This method uses JSON.stringify for comparison, which may not work correctly + * if the properties in the key object are rearranged or reordered. + */ + get(keyObj?: object) { + return JSON.stringify(keyObj) === this.#key + ? this.#cached + : undefined; + } + + set(keyObj: object | undefined, obj: any) { + this.#cached = obj; + this.#key = JSON.stringify(keyObj); + } +} From 4e35ff10da4155e4ef61f2fcfa9f9638fea078d4 Mon Sep 17 00:00:00 2001 From: Nikolay Karadzhov Date: Wed, 14 May 2025 16:30:12 +0300 Subject: [PATCH 2/3] handle circular structures --- .../client/lib/single-entry-cache.spec.ts | 12 ++++++++++ packages/client/lib/single-entry-cache.ts | 23 ++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/packages/client/lib/single-entry-cache.spec.ts b/packages/client/lib/single-entry-cache.spec.ts index ed6ea206a60..ef535738eac 100644 --- a/packages/client/lib/single-entry-cache.spec.ts +++ b/packages/client/lib/single-entry-cache.spec.ts @@ -70,4 +70,16 @@ describe('SingleEntryCache', () => { assert.strictEqual(cache.get(keyObj2), undefined); }); + + it('should handle circular structures', () => { + const keyObj: any = {}; + keyObj.self = keyObj; + + const instance = { data: 'test data' }; + + cache.set(keyObj, instance); + + assert.strictEqual(cache.get(keyObj), instance); + }); + }); diff --git a/packages/client/lib/single-entry-cache.ts b/packages/client/lib/single-entry-cache.ts index c9c6a993466..a7160c032eb 100644 --- a/packages/client/lib/single-entry-cache.ts +++ b/packages/client/lib/single-entry-cache.ts @@ -1,25 +1,36 @@ +function makeCircularReplacer() { + const seen = new WeakSet(); + return function serialize(_: string, value: any) { + if (value && typeof value === 'object') { + if (seen.has(value)) { + return 'circular'; + } + seen.add(value); + return value; + } + return value; + } +} export default class SingleEntryCache { #cached?: any; #key?: string; /** * Retrieves an instance from the cache based on the provided key object. - * + * * @param keyObj - The key object to look up in the cache. * @returns The cached instance if found, undefined otherwise. - * + * * @remarks * This method uses JSON.stringify for comparison, which may not work correctly * if the properties in the key object are rearranged or reordered. */ get(keyObj?: object) { - return JSON.stringify(keyObj) === this.#key - ? this.#cached - : undefined; + return JSON.stringify(keyObj, makeCircularReplacer()) === this.#key ? this.#cached : undefined; } set(keyObj: object | undefined, obj: any) { this.#cached = obj; - this.#key = JSON.stringify(keyObj); + this.#key = JSON.stringify(keyObj, makeCircularReplacer()); } } From 7143297640d91b6571279253d8a618476e537d6d Mon Sep 17 00:00:00 2001 From: Nikolay Karadzhov Date: Wed, 14 May 2025 17:19:37 +0300 Subject: [PATCH 3/3] make cache generic --- packages/client/lib/client/index.ts | 2 +- packages/client/lib/client/pool.ts | 2 +- packages/client/lib/cluster/index.ts | 2 +- packages/client/lib/single-entry-cache.ts | 41 ++++++++++++----------- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/packages/client/lib/client/index.ts b/packages/client/lib/client/index.ts index e4b781fb5aa..f3e72a3a172 100644 --- a/packages/client/lib/client/index.ts +++ b/packages/client/lib/client/index.ts @@ -207,7 +207,7 @@ export default class RedisClient< } } - static #SingleEntryCache = new SingleEntryCache() + static #SingleEntryCache = new SingleEntryCache() static factory< M extends RedisModules = {}, diff --git a/packages/client/lib/client/pool.ts b/packages/client/lib/client/pool.ts index 086d6ba192b..ec89f0c39e3 100644 --- a/packages/client/lib/client/pool.ts +++ b/packages/client/lib/client/pool.ts @@ -111,7 +111,7 @@ export class RedisClientPool< }; } - static #SingleEntryCache = new SingleEntryCache(); + static #SingleEntryCache = new SingleEntryCache(); static create< M extends RedisModules, diff --git a/packages/client/lib/cluster/index.ts b/packages/client/lib/cluster/index.ts index 8ae80c8468f..8b37f9c1aa7 100644 --- a/packages/client/lib/cluster/index.ts +++ b/packages/client/lib/cluster/index.ts @@ -214,7 +214,7 @@ export default class RedisCluster< }; } - static #SingleEntryCache = new SingleEntryCache(); + static #SingleEntryCache = new SingleEntryCache(); static factory< M extends RedisModules = {}, diff --git a/packages/client/lib/single-entry-cache.ts b/packages/client/lib/single-entry-cache.ts index a7160c032eb..5c65df96660 100644 --- a/packages/client/lib/single-entry-cache.ts +++ b/packages/client/lib/single-entry-cache.ts @@ -1,19 +1,6 @@ -function makeCircularReplacer() { - const seen = new WeakSet(); - return function serialize(_: string, value: any) { - if (value && typeof value === 'object') { - if (seen.has(value)) { - return 'circular'; - } - seen.add(value); - return value; - } - return value; - } -} -export default class SingleEntryCache { - #cached?: any; - #key?: string; +export default class SingleEntryCache { + #cached?: V; + #serializedKey?: string; /** * Retrieves an instance from the cache based on the provided key object. @@ -25,12 +12,26 @@ export default class SingleEntryCache { * This method uses JSON.stringify for comparison, which may not work correctly * if the properties in the key object are rearranged or reordered. */ - get(keyObj?: object) { - return JSON.stringify(keyObj, makeCircularReplacer()) === this.#key ? this.#cached : undefined; + get(keyObj?: K): V | undefined { + return JSON.stringify(keyObj, makeCircularReplacer()) === this.#serializedKey ? this.#cached : undefined; } - set(keyObj: object | undefined, obj: any) { + set(keyObj: K | undefined, obj: V) { this.#cached = obj; - this.#key = JSON.stringify(keyObj, makeCircularReplacer()); + this.#serializedKey = JSON.stringify(keyObj, makeCircularReplacer()); } } + +function makeCircularReplacer() { + const seen = new WeakSet(); + return function serialize(_: string, value: any) { + if (value && typeof value === 'object') { + if (seen.has(value)) { + return 'circular'; + } + seen.add(value); + return value; + } + return value; + } +} \ No newline at end of file