do not create a unique connection pool for every CacheService instance

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

.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

.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

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');
+    });
+  });
+});

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,
+      });
   }
 }