From f66bbb408795bf00ff1847c36db13db9db82d366 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Fri, 10 May 2024 12:32:16 +0200 Subject: [PATCH] do not create a unique connection pool for every CacheService instance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/nervous-mayflies-float.md | 5 ++ .github/workflows/ci.yml | 10 +++ .../cache/CacheManager.integration.test.ts | 88 +++++++++++++++++++ .../backend-common/src/cache/CacheManager.ts | 78 ++++++++-------- 4 files changed, 142 insertions(+), 39 deletions(-) create mode 100644 .changeset/nervous-mayflies-float.md create mode 100644 packages/backend-common/src/cache/CacheManager.integration.test.ts diff --git a/.changeset/nervous-mayflies-float.md b/.changeset/nervous-mayflies-float.md new file mode 100644 index 0000000000..e8d29be280 --- /dev/null +++ b/.changeset/nervous-mayflies-float.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Only create a single actual connection to memcache/redis, even in cases where many `CacheService` instances are made diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c538ec8957..ae90ba164b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -187,6 +187,15 @@ jobs: --health-retries 5 ports: - 3306/tcp + redis: + image: redis + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 6379/tcp env: CI: true @@ -231,6 +240,7 @@ jobs: BACKSTAGE_TEST_DATABASE_POSTGRES16_CONNECTION_STRING: postgresql://postgres:postgres@localhost:${{ job.services.postgres16.ports[5432] }} BACKSTAGE_TEST_DATABASE_POSTGRES12_CONNECTION_STRING: postgresql://postgres:postgres@localhost:${{ job.services.postgres12.ports[5432] }} BACKSTAGE_TEST_DATABASE_MYSQL8_CONNECTION_STRING: mysql://root:root@localhost:${{ job.services.mysql8.ports[3306] }}/ignored + BACKSTAGE_TEST_CACHE_REDIS_CONNECTION_STRING: redis://localhost:${{ job.services.redis.ports[6379] }} # We run the test cases before verifying the specs to prevent any failing tests from causing errors. - name: verify openapi specs against test cases diff --git a/packages/backend-common/src/cache/CacheManager.integration.test.ts b/packages/backend-common/src/cache/CacheManager.integration.test.ts new file mode 100644 index 0000000000..7dc60a9051 --- /dev/null +++ b/packages/backend-common/src/cache/CacheManager.integration.test.ts @@ -0,0 +1,88 @@ +/* + * Copyright 2024 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ConfigReader } from '@backstage/config'; +import { CacheManager } from './CacheManager'; +import KeyvRedis from '@keyv/redis'; + +// This test is in a separate file because the main test file uses other mocking +// that might interfere with this one. + +// Contrived code because it's hard to spy on a default export +jest.mock('@keyv/redis', () => { + const ActualKeyvRedis = jest.requireActual('@keyv/redis'); + return jest + .fn() + .mockImplementation((...args: any[]) => new ActualKeyvRedis(...args)); +}); + +describe('CacheManager integration', () => { + describe('redis', () => { + it('only creates one underlying connection', async () => { + const manager = CacheManager.fromConfig( + new ConfigReader({ + backend: { + cache: { + store: 'redis', + // no actual connection errors will be seen since we don't interact with it + connection: 'redis://localhost:6379', + }, + }, + }), + ); + + manager.forPlugin('p1').getClient(); + manager.forPlugin('p1').getClient({ defaultTtl: 200 }); + manager.forPlugin('p2').getClient(); + manager.forPlugin('p3').getClient({}); + + expect(KeyvRedis).toHaveBeenCalledTimes(1); + }); + + it('interacts correctly with redis', async () => { + // TODO(freben): This could be frameworkified as TestCaches just like + // TestDatabases, but that will have to come some other day + const connection = + process.env.BACKSTAGE_TEST_CACHE_REDIS_CONNECTION_STRING; + if (!connection) { + return; + } + + const manager = CacheManager.fromConfig( + new ConfigReader({ + backend: { + cache: { + store: 'redis', + connection, + }, + }, + }), + ); + + const plugin1 = manager.forPlugin('p1').getClient(); + const plugin2a = manager.forPlugin('p2').getClient(); + const plugin2b = manager.forPlugin('p2').getClient({ defaultTtl: 2000 }); + + await plugin1.set('a', 'plugin1'); + await plugin2a.set('a', 'plugin2a'); + await plugin2b.set('a', 'plugin2b'); + + await expect(plugin1.get('a')).resolves.toBe('plugin1'); + await expect(plugin2a.get('a')).resolves.toBe('plugin2b'); + await expect(plugin2b.get('a')).resolves.toBe('plugin2b'); + }); + }); +}); diff --git a/packages/backend-common/src/cache/CacheManager.ts b/packages/backend-common/src/cache/CacheManager.ts index c7298ab3e9..ec8a1d93f7 100644 --- a/packages/backend-common/src/cache/CacheManager.ts +++ b/packages/backend-common/src/cache/CacheManager.ts @@ -27,6 +27,8 @@ import { getRootLogger } from '../logging'; import { DefaultCacheClient } from './CacheClient'; import { CacheManagerOptions, PluginCacheManager } from './types'; +type StoreFactory = (pluginId: string, defaultTtl: number | undefined) => Keyv; + /** * Implements a Cache Manager which will automatically create new cache clients * for plugins when requested. All requested cache clients are created with the @@ -40,18 +42,11 @@ export class CacheManager { * that return Keyv instances appropriate to the store. */ private readonly storeFactories = { - redis: this.getRedisClient, - memcache: this.getMemcacheClient, - memory: this.getMemoryClient, + redis: this.createRedisStoreFactory(), + memcache: this.createMemcacheStoreFactory(), + memory: this.createMemoryStoreFactory(), }; - /** - * Shared memory store for the in-memory cache client. Sharing the same Map - * instance ensures get/set/delete operations hit the same store, regardless - * of where/when a client is instantiated. - */ - private readonly memoryStore = new Map(); - private readonly logger: LoggerService; private readonly store: keyof CacheManager['storeFactories']; private readonly connection: string; @@ -148,41 +143,46 @@ export class CacheManager { } private getClientWithTtl(pluginId: string, ttl: number | undefined): Keyv { - return this.storeFactories[this.store].call(this, pluginId, ttl); + return this.storeFactories[this.store](pluginId, ttl); } - private getRedisClient( - pluginId: string, - defaultTtl: number | undefined, - ): Keyv { - return new Keyv({ - namespace: pluginId, - ttl: defaultTtl, - store: new KeyvRedis(this.connection), - useRedisSets: this.useRedisSets, - }); + private createRedisStoreFactory(): StoreFactory { + let store: KeyvRedis | undefined; + return (pluginId, defaultTtl) => { + if (!store) { + store = new KeyvRedis(this.connection); + } + return new Keyv({ + namespace: pluginId, + ttl: defaultTtl, + store, + useRedisSets: this.useRedisSets, + }); + }; } - private getMemcacheClient( - pluginId: string, - defaultTtl: number | undefined, - ): Keyv { - return new Keyv({ - namespace: pluginId, - ttl: defaultTtl, - store: new KeyvMemcache(this.connection), - }); + private createMemcacheStoreFactory(): StoreFactory { + let store: KeyvMemcache | undefined; + return (pluginId, defaultTtl) => { + if (!store) { + store = new KeyvMemcache(this.connection); + } + return new Keyv({ + namespace: pluginId, + ttl: defaultTtl, + store, + }); + }; } - private getMemoryClient( - pluginId: string, - defaultTtl: number | undefined, - ): Keyv { - return new Keyv({ - namespace: pluginId, - ttl: defaultTtl, - store: this.memoryStore, - }); + private createMemoryStoreFactory(): StoreFactory { + const store = new Map(); + return (pluginId, defaultTtl) => + new Keyv({ + namespace: pluginId, + ttl: defaultTtl, + store, + }); } }