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.

Author: lutovich

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);
 

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 {

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);
   }