From 27fb1232fdd1eb5e4e28da786de441243e71b741 Mon Sep 17 00:00:00 2001 From: Oskar Dudycz Date: Fri, 12 Jul 2024 12:06:56 +0200 Subject: [PATCH 1/2] Fixed connection pool management to correctly handle database names Thanks to that pongoClient.close can close connection pools without needing to call endAllPools. PongoClient will track opened dbClients and close them all. --- src/package-lock.json | 20 ++++++ src/package.json | 2 + src/packages/dumbo/package.json | 7 ++- src/packages/dumbo/src/connections/client.ts | 2 +- .../dumbo/src/connections/connectionString.ts | 6 ++ src/packages/dumbo/src/connections/index.ts | 1 + src/packages/dumbo/src/connections/pool.ts | 62 ++++++++++++++++--- src/packages/pongo/package.json | 10 ++- .../src/e2e/compatibilityTest.e2e.spec.ts | 3 +- src/packages/pongo/src/main/client.ts | 25 ++++++-- src/packages/pongo/src/postgres/client.ts | 2 +- 11 files changed, 120 insertions(+), 20 deletions(-) create mode 100644 src/packages/dumbo/src/connections/connectionString.ts diff --git a/src/package-lock.json b/src/package-lock.json index 5d6ebb06..9aac6248 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -23,6 +23,7 @@ "@types/mongodb": "^4.0.7", "@types/node": "20.11.30", "@types/pg": "^8.11.6", + "@types/pg-connection-string": "^2.0.0", "@types/pg-format": "^1.0.5", "@types/uuid": "9.0.8", "@typescript-eslint/eslint-plugin": "7.9.0", @@ -46,6 +47,7 @@ }, "peerDependencies": { "pg": "^8.12.0", + "pg-connection-string": "^2.6.4", "pg-format": "^1.0.4", "testcontainers": "^10.10.1" } @@ -1350,6 +1352,15 @@ "pg-types": "^4.0.1" } }, + "node_modules/@types/pg-connection-string": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@types/pg-connection-string/-/pg-connection-string-2.0.0.tgz", + "integrity": "sha512-J1ZxH9dN0zSwnJpAlbvRlEtwYD/SYmyLMVF4DF0qfTHv0sH0HEg9WK/B24doWGjgcT8InomL0znJwcYHy3lNUw==", + "deprecated": "This is a stub types definition for pg-connection-string (https://github.com/iceddev/pg-connection-string). pg-connection-string provides its own type definitions, so you don't need @types/pg-connection-string installed!", + "dependencies": { + "pg-connection-string": "*" + } + }, "node_modules/@types/pg-format": { "version": "1.0.5", "resolved": "https://registry.npmjs.org/@types/pg-format/-/pg-format-1.0.5.tgz", @@ -6626,14 +6637,19 @@ "packages/dumbo": { "name": "@event-driven-io/dumbo", "version": "0.1.0", + "dependencies": { + "@event-driven-io/dumbo": "^0.1.0" + }, "devDependencies": { "@types/node": "20.11.30" }, "peerDependencies": { "@types/pg": "^8.11.6", + "@types/pg-connection-string": "^2.0.0", "@types/pg-format": "^1.0.5", "@types/uuid": "^9.0.8", "pg": "^8.12.0", + "pg-connection-string": "^2.6.4", "pg-format": "^1.0.4", "uuid": "^9.0.1" } @@ -6641,6 +6657,10 @@ "packages/pongo": { "name": "@event-driven-io/pongo", "version": "0.3.0", + "dependencies": { + "@types/pg-connection-string": "^2.0.0", + "pg-connection-string": "^2.6.4" + }, "devDependencies": { "@types/node": "20.11.30" }, diff --git a/src/package.json b/src/package.json index 8ddc0c38..18da028f 100644 --- a/src/package.json +++ b/src/package.json @@ -69,6 +69,7 @@ "@types/mongodb": "^4.0.7", "@types/node": "20.11.30", "@types/pg": "^8.11.6", + "@types/pg-connection-string": "^2.0.0", "@types/pg-format": "^1.0.5", "@types/uuid": "9.0.8", "@typescript-eslint/eslint-plugin": "7.9.0", @@ -89,6 +90,7 @@ }, "peerDependencies": { "pg": "^8.12.0", + "pg-connection-string": "^2.6.4", "pg-format": "^1.0.4", "testcontainers": "^10.10.1" }, diff --git a/src/packages/dumbo/package.json b/src/packages/dumbo/package.json index 0b0576f6..c23f5f7e 100644 --- a/src/packages/dumbo/package.json +++ b/src/packages/dumbo/package.json @@ -47,14 +47,19 @@ "dist" ], "peerDependencies": { - "@types/uuid": "^9.0.8", "@types/pg": "^8.11.6", + "@types/pg-connection-string": "^2.0.0", "@types/pg-format": "^1.0.5", + "@types/uuid": "^9.0.8", "pg": "^8.12.0", + "pg-connection-string": "^2.6.4", "pg-format": "^1.0.4", "uuid": "^9.0.1" }, "devDependencies": { "@types/node": "20.11.30" + }, + "dependencies": { + "@event-driven-io/dumbo": "^0.1.0" } } diff --git a/src/packages/dumbo/src/connections/client.ts b/src/packages/dumbo/src/connections/client.ts index 8b9fcf0b..f216c6c0 100644 --- a/src/packages/dumbo/src/connections/client.ts +++ b/src/packages/dumbo/src/connections/client.ts @@ -14,6 +14,6 @@ export const postgresClient = ( return { connect: () => pool.connect(), - close: () => endPool(connectionString), + close: () => endPool({ connectionString, database }), }; }; diff --git a/src/packages/dumbo/src/connections/connectionString.ts b/src/packages/dumbo/src/connections/connectionString.ts new file mode 100644 index 00000000..e817accc --- /dev/null +++ b/src/packages/dumbo/src/connections/connectionString.ts @@ -0,0 +1,6 @@ +import pgcs from 'pg-connection-string'; + +export const defaultPostgreSqlDatabase = 'postgres'; + +export const getDatabaseNameOrDefault = (connectionString: string) => + pgcs.parse(connectionString).database ?? defaultPostgreSqlDatabase; diff --git a/src/packages/dumbo/src/connections/index.ts b/src/packages/dumbo/src/connections/index.ts index b9936bb7..bf76b94f 100644 --- a/src/packages/dumbo/src/connections/index.ts +++ b/src/packages/dumbo/src/connections/index.ts @@ -1,2 +1,3 @@ export * from './client'; +export * from './connectionString'; export * from './pool'; diff --git a/src/packages/dumbo/src/connections/pool.ts b/src/packages/dumbo/src/connections/pool.ts index 96f1ecda..444713fd 100644 --- a/src/packages/dumbo/src/connections/pool.ts +++ b/src/packages/dumbo/src/connections/pool.ts @@ -1,6 +1,11 @@ import pg from 'pg'; +import { + defaultPostgreSqlDatabase, + getDatabaseNameOrDefault, +} from './connectionString'; const pools: Map = new Map(); +const usageCounter: Map = new Map(); export const getPool = ( connectionStringOrOptions: string | pg.PoolConfig, @@ -15,22 +20,63 @@ export const getPool = ( ? { connectionString } : connectionStringOrOptions; - //TODO: this should include database name resolution for key + const database = + poolOptions.database ?? + (poolOptions.connectionString + ? getDatabaseNameOrDefault(poolOptions.connectionString) + : undefined); + + const lookupKey = key(connectionString, database); + + updatePoolUsageCounter(lookupKey, 1); + return ( - pools.get(connectionString) ?? - pools.set(connectionString, new pg.Pool(poolOptions)).get(connectionString)! + pools.get(lookupKey) ?? + pools.set(lookupKey, new pg.Pool(poolOptions)).get(lookupKey)! ); }; -export const endPool = async (connectionString: string): Promise => { - const pool = pools.get(connectionString); - if (pool) { +export const endPool = async ({ + connectionString, + database, + force, +}: { + connectionString: string; + database?: string | undefined; + force?: boolean; +}): Promise => { + database = database ?? getDatabaseNameOrDefault(connectionString); + const lookupKey = key(connectionString, database); + + const pool = pools.get(lookupKey); + if (pool && (updatePoolUsageCounter(lookupKey, -1) <= 0 || force === true)) { + await onEndPool(lookupKey, pool); + } +}; + +export const onEndPool = async (lookupKey: string, pool: pg.Pool) => { + try { await pool.end(); - pools.delete(connectionString); + pools.delete(lookupKey); + } catch (error) { + console.log(`Error while closing the connection pool: ${lookupKey}`); + console.log(error); } }; export const endAllPools = () => Promise.all( - [...pools.keys()].map((connectionString) => endPool(connectionString)), + [...pools.entries()].map(([lookupKey, pool]) => onEndPool(lookupKey, pool)), ); + +const key = (connectionString: string, database: string | undefined) => + `${connectionString}|${database ?? defaultPostgreSqlDatabase}`; + +const updatePoolUsageCounter = (lookupKey: string, by: 1 | -1): number => { + const currentCounter = usageCounter.get(lookupKey) ?? 0; + const newCounter = currentCounter + by; + + usageCounter.set(lookupKey, currentCounter + by); + + return newCounter; +}; diff --git a/src/packages/pongo/package.json b/src/packages/pongo/package.json index 62bb0220..0a3d30f1 100644 --- a/src/packages/pongo/package.json +++ b/src/packages/pongo/package.json @@ -47,16 +47,20 @@ "dist" ], "peerDependencies": { - "@types/uuid": "^9.0.8", + "@event-driven-io/dumbo": "^0.1.0", + "@types/mongodb": "^4.0.7", "@types/pg": "^8.11.6", "@types/pg-format": "^1.0.5", - "@types/mongodb": "^4.0.7", - "@event-driven-io/dumbo": "^0.1.0", + "@types/uuid": "^9.0.8", "pg": "^8.12.0", "pg-format": "^1.0.4", "uuid": "^9.0.1" }, "devDependencies": { "@types/node": "20.11.30" + }, + "dependencies": { + "@types/pg-connection-string": "^2.0.0", + "pg-connection-string": "^2.6.4" } } diff --git a/src/packages/pongo/src/e2e/compatibilityTest.e2e.spec.ts b/src/packages/pongo/src/e2e/compatibilityTest.e2e.spec.ts index 7fb861b2..1078c050 100644 --- a/src/packages/pongo/src/e2e/compatibilityTest.e2e.spec.ts +++ b/src/packages/pongo/src/e2e/compatibilityTest.e2e.spec.ts @@ -1,4 +1,3 @@ -import { endAllPools } from '@event-driven-io/dumbo'; import { MongoDBContainer, type StartedMongoDBContainer, @@ -60,7 +59,7 @@ void describe('MongoDB Compatibility Tests', () => { after(async () => { try { - await endAllPools(); + await pongoClient.close(); await postgres.stop(); } catch (error) { console.log(error); diff --git a/src/packages/pongo/src/main/client.ts b/src/packages/pongo/src/main/client.ts index ebd31de5..9fbf2c1a 100644 --- a/src/packages/pongo/src/main/client.ts +++ b/src/packages/pongo/src/main/client.ts @@ -1,17 +1,34 @@ -import { getDbClient } from './dbClient'; +import { getDatabaseNameOrDefault } from '@event-driven-io/dumbo'; +import { getDbClient, type DbClient } from './dbClient'; import type { PongoClient, PongoDb } from './typing/operations'; export const pongoClient = (connectionString: string): PongoClient => { + const defaultDbName = getDatabaseNameOrDefault(connectionString); + const dbClients: Map = new Map(); + const dbClient = getDbClient(connectionString); + dbClients.set(defaultDbName, dbClient); const pongoClient: PongoClient = { connect: async () => { await dbClient.connect(); return pongoClient; }, - close: () => dbClient.close(), - db: (dbName?: string): PongoDb => - dbName ? getDbClient(connectionString, dbName) : dbClient, + close: async () => { + for (const db of dbClients.values()) { + await db.close(); + } + }, + db: (dbName?: string): PongoDb => { + if (!dbName) return dbClient; + + return ( + dbClients.get(dbName) ?? + dbClients + .set(dbName, getDbClient(connectionString, dbName)) + .get(dbName)! + ); + }, }; return pongoClient; diff --git a/src/packages/pongo/src/postgres/client.ts b/src/packages/pongo/src/postgres/client.ts index d30223e1..23b80d36 100644 --- a/src/packages/pongo/src/postgres/client.ts +++ b/src/packages/pongo/src/postgres/client.ts @@ -10,7 +10,7 @@ export const postgresClient = ( return { connect: () => Promise.resolve(), - close: () => endPool(connectionString), + close: () => endPool({ connectionString, database }), collection: (name: string) => postgresCollection(name, pool), }; }; From 8cef6b5b8ed3a743ffb777795031cd924d6e2f74 Mon Sep 17 00:00:00 2001 From: Oskar Dudycz Date: Fri, 12 Jul 2024 12:43:52 +0200 Subject: [PATCH 2/2] Added option to inject PoolClient to PongoClient That will enable running transactions and sharing connection with Emmett projections --- src/packages/pongo/src/main/dbClient.ts | 9 +++----- src/packages/pongo/src/main/index.ts | 2 +- .../src/main/{client.ts => pongoClient.ts} | 4 ++-- src/packages/pongo/src/postgres/client.ts | 23 +++++++++++++------ .../pongo/src/postgres/postgresCollection.ts | 2 +- 5 files changed, 23 insertions(+), 17 deletions(-) rename src/packages/pongo/src/main/{client.ts => pongoClient.ts} (87%) diff --git a/src/packages/pongo/src/main/dbClient.ts b/src/packages/pongo/src/main/dbClient.ts index 6e79ade3..6cf70f93 100644 --- a/src/packages/pongo/src/main/dbClient.ts +++ b/src/packages/pongo/src/main/dbClient.ts @@ -1,4 +1,4 @@ -import { postgresClient } from '../postgres'; +import { postgresClient, type PongoClientOptions } from '../postgres'; import type { PongoCollection } from './typing/operations'; export interface DbClient { @@ -7,10 +7,7 @@ export interface DbClient { collection: (name: string) => PongoCollection; } -export const getDbClient = ( - connectionString: string, - database?: string, -): DbClient => { +export const getDbClient = (options: PongoClientOptions): DbClient => { // This is the place where in the future could come resolution of other database types - return postgresClient(connectionString, database); + return postgresClient(options); }; diff --git a/src/packages/pongo/src/main/index.ts b/src/packages/pongo/src/main/index.ts index cbc1b7ce..a88ac3a9 100644 --- a/src/packages/pongo/src/main/index.ts +++ b/src/packages/pongo/src/main/index.ts @@ -1,3 +1,3 @@ -export * from './client'; export * from './dbClient'; +export * from './pongoClient'; export * from './typing'; diff --git a/src/packages/pongo/src/main/client.ts b/src/packages/pongo/src/main/pongoClient.ts similarity index 87% rename from src/packages/pongo/src/main/client.ts rename to src/packages/pongo/src/main/pongoClient.ts index 9fbf2c1a..e4154233 100644 --- a/src/packages/pongo/src/main/client.ts +++ b/src/packages/pongo/src/main/pongoClient.ts @@ -6,7 +6,7 @@ export const pongoClient = (connectionString: string): PongoClient => { const defaultDbName = getDatabaseNameOrDefault(connectionString); const dbClients: Map = new Map(); - const dbClient = getDbClient(connectionString); + const dbClient = getDbClient({ connectionString }); dbClients.set(defaultDbName, dbClient); const pongoClient: PongoClient = { @@ -25,7 +25,7 @@ export const pongoClient = (connectionString: string): PongoClient => { return ( dbClients.get(dbName) ?? dbClients - .set(dbName, getDbClient(connectionString, dbName)) + .set(dbName, getDbClient({ connectionString, database: dbName })) .get(dbName)! ); }, diff --git a/src/packages/pongo/src/postgres/client.ts b/src/packages/pongo/src/postgres/client.ts index 23b80d36..a7d1fdfc 100644 --- a/src/packages/pongo/src/postgres/client.ts +++ b/src/packages/pongo/src/postgres/client.ts @@ -1,16 +1,25 @@ import { endPool, getPool } from '@event-driven-io/dumbo'; +import pg from 'pg'; import { type DbClient } from '../main'; import { postgresCollection } from './postgresCollection'; -export const postgresClient = ( - connectionString: string, - database?: string, -): DbClient => { - const pool = getPool({ connectionString, database }); +export type PongoClientOptions = { + connectionString: string; + database?: string | undefined; + client?: pg.PoolClient; +}; + +export const postgresClient = (options: PongoClientOptions): DbClient => { + const { connectionString, database, client } = options; + const managesPoolLifetime = !client; + const clientOrPool = client ?? getPool({ connectionString, database }); return { connect: () => Promise.resolve(), - close: () => endPool({ connectionString, database }), - collection: (name: string) => postgresCollection(name, pool), + close: () => + managesPoolLifetime + ? endPool({ connectionString, database }) + : Promise.resolve(), + collection: (name: string) => postgresCollection(name, clientOrPool), }; }; diff --git a/src/packages/pongo/src/postgres/postgresCollection.ts b/src/packages/pongo/src/postgres/postgresCollection.ts index a93c76fd..1dc5538e 100644 --- a/src/packages/pongo/src/postgres/postgresCollection.ts +++ b/src/packages/pongo/src/postgres/postgresCollection.ts @@ -17,7 +17,7 @@ import { buildUpdateQuery } from './update'; export const postgresCollection = ( collectionName: string, - pool: pg.Pool, + pool: pg.Pool | pg.PoolClient, ): PongoCollection => { const execute = (sql: SQL) => executeSQL(pool, sql); const SqlFor = collectionSQLBuilder(collectionName);