fix(pool): getConnection callback should be called only once

Author: fenying

lib/pool.js

@@ -56,7 +56,7 @@ class Pool extends EventEmitter {
         config: this.config.connectionConfig
       });
       this._allConnections.push(connection);
-      return connection.connect(err => {
+      return connection.connect((err) => {
         if (this._closed) {
           return cb(new Error('Pool is closed.'));
         }
@@ -80,21 +80,20 @@ class Pool extends EventEmitter {
     this.emit('enqueue');
 
     if (this.config.acquireTimeout > 0) {
+      // eslint-disable-next-line prefer-const
+      let timer;
 
-      const timeout = setTimeout(() => {
+      const wrappedCb = (err, connection) => {
+        clearTimeout(timer);
+        cb(err, connection);
+      };
 
-        spliceConnection(this._connectionQueue, cb);
+      timer = setTimeout(() => {
+        spliceConnection(this._connectionQueue, wrappedCb);
         cb(new Error('Timeout acquiring connection'));
-
       }, this.config.acquireTimeout);
 
-      return this._connectionQueue.push((err, connection) => {
-
-        clearTimeout(timeout);
-
-        cb(err, connection);
-
-      }, this.config.acquireTimeout);
+      return this._connectionQueue.push(wrappedCb);
     }
 
     return this._connectionQueue.push(cb);
@@ -121,7 +120,7 @@ class Pool extends EventEmitter {
     this._closed = true;
     clearTimeout(this._removeIdleTimeoutConnectionsTimer);
     if (typeof cb !== 'function') {
-      cb = function(err) {
+      cb = function (err) {
         if (err) {
           throw err;
         }
@@ -130,7 +129,7 @@ class Pool extends EventEmitter {
     let calledBack = false;
     let closedConnections = 0;
     let connection;
-    const endCB = function(err) {
+    const endCB = function (err) {
       if (calledBack) {
         return;
       }
@@ -158,7 +157,8 @@ class Pool extends EventEmitter {
       this.config.connectionConfig
     );
     if (typeof cmdQuery.namedPlaceholders === 'undefined') {
-      cmdQuery.namedPlaceholders = this.config.connectionConfig.namedPlaceholders;
+      cmdQuery.namedPlaceholders =
+        this.config.connectionConfig.namedPlaceholders;
     }
     this.getConnection((err, conn) => {
       if (err) {

test/integration/promise-wrappers/test-promise-wrappers.test.cjs

@@ -75,6 +75,52 @@ function testErrors() {
     });
 }
 
+function testPoolAcquireTimeout() {
+
+  (async () => {
+    const ACQUIRE_TIMEOUT = 500;
+    const pool = createPool({
+      ...config,
+      connectionLimit: 2,
+      acquireTimeout: ACQUIRE_TIMEOUT,
+    });
+
+    const c1 = await pool.getConnection();
+
+    assert.ok(c1);
+
+    const c2 = await pool.getConnection();
+
+    assert.ok(c2);
+
+    const c3StartedAt = Date.now();
+    try {
+
+      await pool.getConnection();
+      assert.fail('should not reach here');
+    }
+    catch (e) {
+
+      assert.equal(Date.now() - c3StartedAt >= ACQUIRE_TIMEOUT, true);
+      assert.equal(e.message, 'Timeout acquiring connection');
+    }
+
+    c2.release();
+
+    const c4 = await pool.getConnection();
+
+    c1.release();
+    c4.release();
+
+    await pool.end();
+
+    assert.ok(c4, 'acquireTimeout is working correctly');
+
+  })().catch((err) => {
+    assert.fail(err);
+  });
+}
+
 function testObjParams() {
   let connResolved;
   createConnection(config)
@@ -473,6 +519,7 @@ testChangeUser();
 testConnectionProperties();
 testPoolConnectionDestroy();
 testPromiseLibrary();
+testPoolAcquireTimeout();
 
 process.on('exit', () => {
   assert.equal(doneCalled, true, 'done not called');

test/integration/test-pool-acquire-timeout.js

@@ -0,0 +1,46 @@
+'use strict';
+
+const mysql = require('../..');
+const assert = require('assert');
+const common = require('../common.test.cjs');
+
+const poolConfig = common.getConfig();
+
+const ACQUIRE_TIMEOUT = 500;
+const pool = new mysql.createPool({
+  ...poolConfig,
+  acquireTimeout: ACQUIRE_TIMEOUT,
+  connectionLimit: 2
+});
+
+pool.getConnection((err, c1) => {
+  assert.equal(!!c1, true);
+  assert.ifError(err);
+
+  pool.getConnection((err, c2) => {
+    assert.ifError(err);
+    assert.equal(!!c2, true);
+
+    const C3_STARTED_AT = Date.now();
+
+    pool.getConnection((e3, c3) => {
+      const C3_DONE_AT = Date.now();
+      assert.equal(C3_DONE_AT - C3_STARTED_AT >= ACQUIRE_TIMEOUT, true);
+      assert.equal(C3_DONE_AT - C3_STARTED_AT < ACQUIRE_TIMEOUT * 2, true);
+
+      assert.notEqual(e3, null);
+      assert.equal(e3.message, 'Timeout acquiring connection', 'Acquire timeout error message is correct');
+      assert.equal(!c3, true);
+      c1.release();
+
+      pool.getConnection((e4, c4) => {
+        assert.equal(e4, null);
+        assert.equal(!!c4, true);
+
+        c4.release();
+        c2.release();
+        pool.end();
+      });
+    });
+  });
+});