From d9924097c5c93e74c44f4279b99adec06f7213b8 Mon Sep 17 00:00:00 2001
From: lutovich <konstantin.lutovich@neotechnology.com>
Date: Mon, 18 Dec 2017 12:33:03 +0100
Subject: [PATCH] Fixed timeout when acquiring connection from pool

Connection pool has limited size and returns `Promise<Connection>`
instead of `Connection` directly. Returned promise can be failed when
connection acquisition timeout has fired. Pool uses helper function
`util#promiseOrTimeout()` to race acquisition and timeout promises.

This function allowed both timeout callback to be invoked and connection
to be acquired. It cleared the timeout outside of the raced promises. As
result connection was acquired but timeout callback failed in background.
It tried to clear non-existing pending acquisition attempts.

This commit fixes the problem by including `#clearTimeout()` call in
the chain of raced promises.
---
 src/v1/internal/pool.js    |  4 ++--
 src/v1/internal/util.js    | 20 ++++++++++++++++---
 test/internal/util.test.js | 41 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/src/v1/internal/pool.js b/src/v1/internal/pool.js
index 355af6d6c..600d1b999 100644
--- a/src/v1/internal/pool.js
+++ b/src/v1/internal/pool.js
@@ -162,10 +162,10 @@ class Pool {
     // check if there are any pending requests
     const requests = this._acquireRequests[key];
     if (requests) {
-      var pending = requests.shift();
+      const pending = requests.shift();
 
       if (pending) {
-        var resource = this._acquire(key);
+        const resource = this._acquire(key);
         if (resource) {
           pending.resolve(resource);
 
diff --git a/src/v1/internal/util.js b/src/v1/internal/util.js
index f1afd856d..88a50b9ed 100644
--- a/src/v1/internal/util.js
+++ b/src/v1/internal/util.js
@@ -17,7 +17,7 @@
  * limitations under the License.
  */
 
-import { newError } from "../error";
+import {newError} from '../error';
 
 const ENCRYPTION_ON = "ENCRYPTION_ON";
 const ENCRYPTION_OFF = "ENCRYPTION_OFF";
@@ -125,6 +125,8 @@ function trimAndVerify(string, name, url) {
 }
 
 function promiseOrTimeout(timeout, otherPromise, onTimeout) {
+  let resultPromise = null;
+
   const timeoutPromise = new Promise((resolve, reject) => {
     const id = setTimeout(() => {
       if (onTimeout && typeof onTimeout === 'function') {
@@ -134,10 +136,22 @@ function promiseOrTimeout(timeout, otherPromise, onTimeout) {
       reject(newError(`Operation timed out in ${timeout} ms.`));
     }, timeout);
 
-    otherPromise.then(() => clearTimeout(id), () => clearTimeout(id));
+    // this "executor" function is executed immediately, even before the Promise constructor returns
+    // thus it's safe to initialize resultPromise variable here, where timeout id variable is accessible
+    resultPromise = otherPromise.then(result => {
+      clearTimeout(id);
+      return result;
+    }).catch(error => {
+      clearTimeout(id);
+      throw error;
+    });
   });
 
-  return Promise.race([ otherPromise, timeoutPromise ]);
+  if (resultPromise == null) {
+    throw new Error('Result promise not initialized');
+  }
+
+  return Promise.race([resultPromise, timeoutPromise]);
 }
 
 export {
diff --git a/test/internal/util.test.js b/test/internal/util.test.js
index c63e307e9..7b432a053 100644
--- a/test/internal/util.test.js
+++ b/test/internal/util.test.js
@@ -204,6 +204,47 @@ describe('util', () => {
     });
   });
 
+  it('should not trigger both promise and timeout', done => {
+    const timeout = 500;
+
+    let timeoutFired = false;
+    let result = null;
+    let error = null;
+
+    const resultPromise = util.promiseOrTimeout(
+      timeout,
+      new Promise(resolve => {
+        setTimeout(() => {
+          resolve(42);
+        }, timeout);
+      }),
+      () => {
+        timeoutFired = true;
+      }
+    );
+
+    resultPromise.then(r => {
+      result = r;
+    }).catch(e => {
+      error = e;
+    });
+
+    setTimeout(() => {
+      if (timeoutFired) {
+        // timeout fired - result should not be set, error should be set
+        expect(result).toBeNull();
+        expect(error).not.toBeNull();
+        expect(error.message).toEqual(`Operation timed out in ${timeout} ms.`);
+        done();
+      } else {
+        // timeout did not fire - result should be set, error should not be set
+        expect(result).toEqual(42);
+        expect(error).toBeNull();
+        done();
+      }
+    }, timeout * 2);
+  });
+
   function verifyValidString(str) {
     expect(util.assertString(str, 'Test string')).toBe(str);
   }