Merge pull request #313 from lutovich/1.5-pool-fix

Fixed timeout when acquiring connection from pool

Author: ali-ince

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