From ac8a72ed3b2c74a6b51f38dd017b71082495a53c Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 3 Oct 2022 14:35:28 +0200 Subject: [PATCH 1/3] chore: cache the database calls and return the cached one if available Signed-off-by: blam --- .../src/database/DatabaseManager.test.ts | 15 ++++++++++++++- .../src/database/DatabaseManager.ts | 6 +++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/database/DatabaseManager.test.ts b/packages/backend-common/src/database/DatabaseManager.test.ts index 9e8af44fc0..8ecb5e4ffb 100644 --- a/packages/backend-common/src/database/DatabaseManager.test.ts +++ b/packages/backend-common/src/database/DatabaseManager.test.ts @@ -113,7 +113,11 @@ describe('DatabaseManager', () => { }, }, }; - const manager = DatabaseManager.fromConfig(new ConfigReader(config)); + let manager: DatabaseManager; + + beforeEach(() => { + manager = DatabaseManager.fromConfig(new ConfigReader(config)); + }); it('connects to a plugin database using default config', async () => { const pluginId = 'pluginwithoutconfig'; @@ -340,6 +344,15 @@ describe('DatabaseManager', () => { ); }); + it('returns the same client for the same pluginId', async () => { + const client1 = await manager.forPlugin('plugin1').getClient(); + const client2 = await manager.forPlugin('plugin1').getClient(); + + expect(mocked(createDatabaseClient)).toHaveBeenCalledTimes(1); + + expect(client1).toBe(client2); + }); + it('uses plugin connection as base if default client is different from plugin client', async () => { const pluginId = 'differentclient'; await manager.forPlugin(pluginId).getClient(); diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index 363bcac30b..4a677a48a7 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -84,6 +84,7 @@ export class DatabaseManager { private readonly config: Config, private readonly prefix: string = 'backstage_plugin_', private readonly options?: DatabaseManagerOptions, + private readonly databaseCache: Map = new Map(), ) {} /** @@ -307,6 +308,9 @@ export class DatabaseManager { * plugin */ private async getDatabase(pluginId: string): Promise { + if (this.databaseCache.has(pluginId)) { + return this.databaseCache.get(pluginId)!; + } const pluginConfig = new ConfigReader( this.getConfigForPlugin(pluginId) as JsonObject, ); @@ -344,7 +348,7 @@ export class DatabaseManager { const client = createDatabaseClient(pluginConfig, databaseClientOverrides); this.startKeepaliveLoop(pluginId, client); - + this.databaseCache.set(pluginId, client); return client; } From c31f7cdfbc78c60b24c330832e59350ecb9d7522 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 3 Oct 2022 14:36:42 +0200 Subject: [PATCH 2/3] chore: added changeset Signed-off-by: blam --- .changeset/plenty-laws-end.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/plenty-laws-end.md diff --git a/.changeset/plenty-laws-end.md b/.changeset/plenty-laws-end.md new file mode 100644 index 0000000000..e026ca3fca --- /dev/null +++ b/.changeset/plenty-laws-end.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Fixed an issue where `getClient()` for a `pluginId` would return different clients and not share them From dcedf1bab5e4dd15186446f794b91a6cf6bfe669 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 3 Oct 2022 15:15:23 +0200 Subject: [PATCH 3/3] chore: store the promise instead to combat race conditions Signed-off-by: blam --- .../src/database/DatabaseManager.test.ts | 7 +- .../src/database/DatabaseManager.ts | 77 +++++++++++-------- 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/packages/backend-common/src/database/DatabaseManager.test.ts b/packages/backend-common/src/database/DatabaseManager.test.ts index 8ecb5e4ffb..95511d5aba 100644 --- a/packages/backend-common/src/database/DatabaseManager.test.ts +++ b/packages/backend-common/src/database/DatabaseManager.test.ts @@ -345,9 +345,10 @@ describe('DatabaseManager', () => { }); it('returns the same client for the same pluginId', async () => { - const client1 = await manager.forPlugin('plugin1').getClient(); - const client2 = await manager.forPlugin('plugin1').getClient(); - + const [client1, client2] = await Promise.all([ + manager.forPlugin('plugin1').getClient(), + manager.forPlugin('plugin1').getClient(), + ]); expect(mocked(createDatabaseClient)).toHaveBeenCalledTimes(1); expect(client1).toBe(client2); diff --git a/packages/backend-common/src/database/DatabaseManager.ts b/packages/backend-common/src/database/DatabaseManager.ts index 4a677a48a7..a58799a724 100644 --- a/packages/backend-common/src/database/DatabaseManager.ts +++ b/packages/backend-common/src/database/DatabaseManager.ts @@ -84,7 +84,7 @@ export class DatabaseManager { private readonly config: Config, private readonly prefix: string = 'backstage_plugin_', private readonly options?: DatabaseManagerOptions, - private readonly databaseCache: Map = new Map(), + private readonly databaseCache: Map> = new Map(), ) {} /** @@ -311,45 +311,58 @@ export class DatabaseManager { if (this.databaseCache.has(pluginId)) { return this.databaseCache.get(pluginId)!; } - const pluginConfig = new ConfigReader( - this.getConfigForPlugin(pluginId) as JsonObject, - ); - const databaseName = this.getDatabaseName(pluginId); - if (databaseName && this.getEnsureExistsConfig(pluginId)) { + const clientPromise = new Promise(async (resolve, reject) => { try { - await ensureDatabaseExists(pluginConfig, databaseName); - } catch (error) { - throw new Error( - `Failed to connect to the database to make sure that '${databaseName}' exists, ${error}`, + const pluginConfig = new ConfigReader( + this.getConfigForPlugin(pluginId) as JsonObject, ); - } - } - let schemaOverrides; - if (this.getPluginDivisionModeConfig() === 'schema') { - schemaOverrides = this.getSchemaOverrides(pluginId); - if (this.getEnsureExistsConfig(pluginId)) { - try { - await ensureSchemaExists(pluginConfig, pluginId); - } catch (error) { - throw new Error( - `Failed to connect to the database to make sure that schema for plugin '${pluginId}' exists, ${error}`, - ); + const databaseName = this.getDatabaseName(pluginId); + if (databaseName && this.getEnsureExistsConfig(pluginId)) { + try { + await ensureDatabaseExists(pluginConfig, databaseName); + } catch (error) { + throw new Error( + `Failed to connect to the database to make sure that '${databaseName}' exists, ${error}`, + ); + } } + + let schemaOverrides; + if (this.getPluginDivisionModeConfig() === 'schema') { + schemaOverrides = this.getSchemaOverrides(pluginId); + if (this.getEnsureExistsConfig(pluginId)) { + try { + await ensureSchemaExists(pluginConfig, pluginId); + } catch (error) { + throw new Error( + `Failed to connect to the database to make sure that schema for plugin '${pluginId}' exists, ${error}`, + ); + } + } + } + + const databaseClientOverrides = mergeDatabaseConfig( + {}, + this.getDatabaseOverrides(pluginId), + schemaOverrides, + ); + + const client = createDatabaseClient( + pluginConfig, + databaseClientOverrides, + ); + this.startKeepaliveLoop(pluginId, client); + resolve(client); + } catch (e) { + reject(e); } - } + }); - const databaseClientOverrides = mergeDatabaseConfig( - {}, - this.getDatabaseOverrides(pluginId), - schemaOverrides, - ); + this.databaseCache.set(pluginId, clientPromise); - const client = createDatabaseClient(pluginConfig, databaseClientOverrides); - this.startKeepaliveLoop(pluginId, client); - this.databaseCache.set(pluginId, client); - return client; + return clientPromise; } private startKeepaliveLoop(pluginId: string, client: Knex): void {