From 50e29b08998b74a13d22b421dd373538fa9ed707 Mon Sep 17 00:00:00 2001 From: MaxAake <61233757+MaxAake@users.noreply.github.com> Date: Thu, 5 Sep 2024 09:28:48 +0200 Subject: [PATCH 1/3] triggers the effects of invalid on error then rethrows Also includes a test case for this. But the pool seems to break after a throw occurs --- packages/core/src/internal/pool/pool.ts | 8 +-- packages/core/test/internal/pool/pool.test.ts | 61 ++++++++++++++++++- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/packages/core/src/internal/pool/pool.ts b/packages/core/src/internal/pool/pool.ts index 18e222c1e..06920c176 100644 --- a/packages/core/src/internal/pool/pool.ts +++ b/packages/core/src/internal/pool/pool.ts @@ -262,10 +262,10 @@ class Pool { try { valid = await this._validateOnAcquire(acquisitionContext, resource) } catch (e) { - if (this._log.isErrorEnabled()) { - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - this._log.error(`Failure on validate ${resource}. This is a bug, please report it. Caused by: ${e.message}`) - } + resourceReleased(key, this._activeResourceCounts) + pool.removeInUse(resource) + await this._destroy(resource) + throw e } if (valid) { diff --git a/packages/core/test/internal/pool/pool.test.ts b/packages/core/test/internal/pool/pool.test.ts index 2c320fcc8..3ab829b39 100644 --- a/packages/core/test/internal/pool/pool.test.ts +++ b/packages/core/test/internal/pool/pool.test.ts @@ -970,8 +970,7 @@ describe('#unit Pool', () => { }) it.each([ - ['is not valid', (promise: any) => promise.resolve(false)], - ['validation fails', (promise: any) => promise.reject(new Error('failed'))] + ['is not valid', (promise: any) => promise.resolve(false)] ])('should create new connection if the current one when %s', async (_, resolver) => { const conns: any[] = [] const pool = new Pool({ @@ -1038,6 +1037,64 @@ describe('#unit Pool', () => { expect(conns.length).toEqual(2) }) + it('should create new connection if the current one when validation fails', async () => { + const conns: any[] = [] + const pool = new Pool({ + // Hook into connection creation to track when and what connections that are + // created. + create: async (_, server, release) => { + // Create a fake connection that makes it possible control when it's connected + // and released from the outer scope. + const conn: any = { + server, + release + } + conns.push(conn) + return conn + }, + validateOnAcquire: async (context, resource: any) => { + const promise = new Promise((resolve, reject) => { + if (resource.promises == null) { + resource.promises = [] + } + resource.promises.push({ + resolve, + reject + }) + }) + + return await promise + }, + // Setup pool to only allow one connection + config: new PoolConfig(1, 100000) + }) + // Make the first request for a connection, this will return a connection instantaneously + const conn0 = await pool.acquire({}, address) + expect(conns.length).toEqual(1) + + // Releasing connection back to the pool, so it can be re-acquired. + await conn0.release(address, conn0) + + // Request the same connection again, it will wait until resolve get called. + const req0 = pool.acquire({}, address) + expect(conns.length).toEqual(1) + + // should resolve the promise with the configured value + conns[0].promises[0].reject(new Error('Failed')) + await expect(async () => await req0).rejects.toThrow() + + // Request other connection, this should also resolve the same connection. + await pool.acquire({}, address) + expect(conns.length).toEqual(1) + + // connection 2 is valid + // conns[0].promises[0].resolve(true) + // getting the connection 2 + // const conn2 = await req1 + // await conn2.release(address, conn2) + // expect(conns.length).toEqual(1) + }, 10000) + it('should not time out if max pool size is not set', async () => { let counter = 0 From 0de094a2a9272cad7e18f4c6ba4373d1ca816210 Mon Sep 17 00:00:00 2001 From: MaxAake <61233757+MaxAake@users.noreply.github.com> Date: Fri, 6 Sep 2024 08:44:21 +0200 Subject: [PATCH 2/3] Improved and working test for validation error Test for errors during validation now functions properly --- packages/core/test/internal/pool/pool.test.ts | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/core/test/internal/pool/pool.test.ts b/packages/core/test/internal/pool/pool.test.ts index 3ab829b39..29721c6cf 100644 --- a/packages/core/test/internal/pool/pool.test.ts +++ b/packages/core/test/internal/pool/pool.test.ts @@ -969,9 +969,7 @@ describe('#unit Pool', () => { await conn2.release(address, conn2) }) - it.each([ - ['is not valid', (promise: any) => promise.resolve(false)] - ])('should create new connection if the current one when %s', async (_, resolver) => { + it('should create new connection if the current one breaks due to being invalid', async () => { const conns: any[] = [] const pool = new Pool({ // Hook into connection creation to track when and what connections that are @@ -1019,7 +1017,7 @@ describe('#unit Pool', () => { expect(conns.length).toEqual(1) // should resolve the promise with the configured value - resolver(conns[0].promises[0]) + conns[0].promises[0].resolve(false) // getting the connection 1 const conn1 = await req0 @@ -1037,7 +1035,7 @@ describe('#unit Pool', () => { expect(conns.length).toEqual(2) }) - it('should create new connection if the current one when validation fails', async () => { + it('should create new connection if the current one breaks from error during validation', async () => { const conns: any[] = [] const pool = new Pool({ // Hook into connection creation to track when and what connections that are @@ -1084,16 +1082,15 @@ describe('#unit Pool', () => { await expect(async () => await req0).rejects.toThrow() // Request other connection, this should also resolve the same connection. - await pool.acquire({}, address) - expect(conns.length).toEqual(1) + const conn2 = await pool.acquire({}, address) + expect(conns.length).toEqual(2) - // connection 2 is valid - // conns[0].promises[0].resolve(true) - // getting the connection 2 - // const conn2 = await req1 - // await conn2.release(address, conn2) - // expect(conns.length).toEqual(1) - }, 10000) + await conn2.release(address, conn2) + expect(conns.length).toEqual(2) + expect(conn0).not.toBe(conn2) + expect(idleResources(pool, address)).toBe(1) + expect(resourceInUse(pool, address)).toBe(0) + }) it('should not time out if max pool size is not set', async () => { let counter = 0 From 70897b9178c882d6dc0e3a88c11fd303e59b2334 Mon Sep 17 00:00:00 2001 From: MaxAake <61233757+MaxAake@users.noreply.github.com> Date: Fri, 6 Sep 2024 09:04:35 +0200 Subject: [PATCH 3/3] run deno build --- packages/neo4j-driver-deno/lib/core/internal/pool/pool.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/neo4j-driver-deno/lib/core/internal/pool/pool.ts b/packages/neo4j-driver-deno/lib/core/internal/pool/pool.ts index 0b46358d5..ecf4f1085 100644 --- a/packages/neo4j-driver-deno/lib/core/internal/pool/pool.ts +++ b/packages/neo4j-driver-deno/lib/core/internal/pool/pool.ts @@ -262,10 +262,10 @@ class Pool { try { valid = await this._validateOnAcquire(acquisitionContext, resource) } catch (e) { - if (this._log.isErrorEnabled()) { - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - this._log.error(`Failure on validate ${resource}. This is a bug, please report it. Caused by: ${e.message}`) - } + resourceReleased(key, this._activeResourceCounts) + pool.removeInUse(resource) + await this._destroy(resource) + throw e } if (valid) {