Throw error from validation failure (#1220)

MaxAake · web-flow · commit 5f5cd5c836f0 · 2024-09-06T14:26:00.000+02:00
Changes the connection pool behavior on errors during validation, these would be unintended behavior and as such the driver should throw an error if it occurs, rather than simply logging as was the case in the 5.24.1 patch release fix.

Also updates the test to control for this behavior.
diff --git a/packages/core/src/internal/pool/pool.ts b/packages/core/src/internal/pool/pool.ts
@@ -262,10 +262,10 @@ class Pool<R extends unknown = unknown> {
         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
@@ -969,10 +969,7 @@ describe('#unit Pool', () => {
     await conn2.release(address, conn2)
   })
 
-  it.each([
-    ['is not valid', (promise: any) => promise.resolve(false)],
-    ['validation fails', (promise: any) => promise.reject(new Error('failed'))]
-  ])('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<any>({
       // Hook into connection creation to track when and what connections that are
@@ -1020,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
@@ -1038,6 +1035,63 @@ describe('#unit Pool', () => {
     expect(conns.length).toEqual(2)
   })
 
+  it('should create new connection if the current one breaks from error during validation', async () => {
+    const conns: any[] = []
+    const pool = new Pool<any>({
+      // 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<boolean>((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.
+    const conn2 = await pool.acquire({}, address)
+    expect(conns.length).toEqual(2)
+
+    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
 
diff --git 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<R extends unknown = unknown> {
         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) {
