From 49e890697bf8b416326aaf74cefa76a8785921d9 Mon Sep 17 00:00:00 2001
From: Peter Wilhelmsson <peter.wilhelmsson@neo4j.com>
Date: Thu, 19 Mar 2020 13:03:21 +0100
Subject: [PATCH] Count pending connects when considering max pool size

Without this there could be a burst of connect and neogitations with the
server on connections just to be shut down immediately.
---
 src/internal/pool.js       | 40 +++++++++++++++++++-----------
 test/internal/pool.test.js | 51 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 76 insertions(+), 15 deletions(-)

diff --git a/src/internal/pool.js b/src/internal/pool.js
index 514f2cdc4..507c54a3f 100644
--- a/src/internal/pool.js
+++ b/src/internal/pool.js
@@ -56,6 +56,7 @@ class Pool {
     this._maxSize = config.maxSize
     this._acquisitionTimeout = config.acquisitionTimeout
     this._pools = {}
+    this._pendingCreates = {}
     this._acquireRequests = {}
     this._activeResourceCounts = {}
     this._release = this._release.bind(this)
@@ -73,18 +74,11 @@ class Pool {
       const key = address.asKey()
 
       if (resource) {
-        if (
-          this._maxSize &&
-          this.activeResourceCount(address) >= this._maxSize
-        ) {
-          this._destroy(resource)
-        } else {
-          resourceAcquired(key, this._activeResourceCounts)
-          if (this._log.isDebugEnabled()) {
-            this._log.debug(`${resource} acquired from the pool ${key}`)
-          }
-          return resource
+        resourceAcquired(key, this._activeResourceCounts)
+        if (this._log.isDebugEnabled()) {
+          this._log.debug(`${resource} acquired from the pool ${key}`)
         }
+        return resource
       }
 
       // We're out of resources and will try to acquire later on when an existing resource is released.
@@ -185,6 +179,7 @@ class Pool {
     if (!pool) {
       pool = []
       this._pools[key] = pool
+      this._pendingCreates[key] = 0
     }
     while (pool.length) {
       const resource = pool.pop()
@@ -201,12 +196,29 @@ class Pool {
       }
     }
 
-    if (this._maxSize && this.activeResourceCount(address) >= this._maxSize) {
-      return null
+    // Ensure requested max pool size
+    if (this._maxSize > 0) {
+      // Include pending creates when checking pool size since these probably will add
+      // to the number when fulfilled.
+      const numConnections =
+        this.activeResourceCount(address) + this._pendingCreates[key]
+      if (numConnections >= this._maxSize) {
+        // Will put this request in queue instead since the pool is full
+        return null
+      }
     }
 
     // there exist no idle valid resources, create a new one for acquisition
-    return await this._create(address, this._release)
+    // Keep track of how many pending creates there are to avoid making too many connections.
+    this._pendingCreates[key] = this._pendingCreates[key] + 1
+    let resource
+    try {
+      // Invoke callback that creates actual connection
+      resource = await this._create(address, this._release)
+    } finally {
+      this._pendingCreates[key] = this._pendingCreates[key] - 1
+    }
+    return resource
   }
 
   async _release (address, resource) {
diff --git a/test/internal/pool.test.js b/test/internal/pool.test.js
index 1554e2b00..6835b5f21 100644
--- a/test/internal/pool.test.js
+++ b/test/internal/pool.test.js
@@ -562,10 +562,59 @@ describe('#unit Pool', async () => {
     expectNumberOfAcquisitionRequests(pool, address, 0)
   })
 
+  const address = ServerAddress.fromUrl('bolt://localhost:7687')
+
+  it('should consider pending connects when evaluating max pool size', async () => {
+    const conns = []
+    const pool = new Pool({
+      // Hook into connection creation to track when and what connections that are
+      // created.
+      create: (server, release) => {
+        // Create a fake connection that makes it possible control when it's connected
+        // and released from the outer scope.
+        const conn = {
+          server: server,
+          release: release
+        }
+        const promise = new Promise((resolve, reject) => {
+          conn.resolve = resolve
+          conn.reject = reject
+        })
+        // Put the connection in a list in outer scope even though there only should be
+        // one when the test is succeeding.
+        conns.push(conn)
+        return promise
+      },
+      // Setup pool to only allow one connection
+      config: new PoolConfig(1, 100000)
+    })
+
+    // Make the first request for a connection, this will be hanging waiting for the
+    // connect promise to be resolved.
+    const req1 = pool.acquire(address)
+    expect(conns.length).toEqual(1)
+
+    // Make another request to the same server, this should not try to acquire another
+    // connection since the pool will be full when the connection for the first request
+    // is resolved.
+    const req2 = pool.acquire(address)
+    expect(conns.length).toEqual(1)
+
+    // Let's fulfill the connect promise belonging to the first request.
+    conns[0].resolve(conns[0])
+    await expectAsync(req1).toBeResolved()
+
+    // Release the connection, it should be picked up by the second request.
+    conns[0].release(address, conns[0])
+    await expectAsync(req2).toBeResolved()
+
+    // Just to make sure that there hasn't been any new connection.
+    expect(conns.length).toEqual(1)
+  })
+
   it('should not time out if max pool size is not set', async () => {
     let counter = 0
 
-    const address = ServerAddress.fromUrl('bolt://localhost:7687')
     const pool = new Pool({
       create: (server, release) =>
         Promise.resolve(new Resource(server, counter++, release)),