refactor(abstract/connection-manager) (#8330)

Resolves issue with acquire call which never finishes, resolve errors and connections and handle factory level error ourselves. Fixed issue with MSSQL / MySQL error out with resources currently not part of pool.

Fixed evict setting so idle connections are properly removed on time and improved documentation around connection manager and various options 

docker-compose.yml

@@ -6,6 +6,7 @@ services:
     links:
      - mysql-57
      - postgres-95
+     - mssql
     volumes:
       - .:/sequelize
     environment:
@@ -39,3 +40,13 @@ services:
     ports:
       - "127.0.0.1:8999:3306"
     container_name: mysql-57
+
+  # MSSQL
+  mssql:
+    image: microsoft/mssql-server-linux:latest
+    environment:
+      ACCEPT_EULA: "Y"
+      SA_PASSWORD: yourStrong(!)Password
+    ports:
+      - "127.0.0.1:8997:1433"
+    container_name: mssql

lib/dialects/mssql/connection-manager.js

@@ -111,9 +111,9 @@ class ConnectionManager extends AbstractConnectionManager {
             break;
         }
       });
-      
+
       if (config.dialectOptions && config.dialectOptions.debug) {
-        connection.on('debug', debugTedious);        
+        connection.on('debug', debugTedious);
       }
 
       if (config.pool.handleDisconnects) {
@@ -121,7 +121,8 @@ class ConnectionManager extends AbstractConnectionManager {
           switch (err.code) {
             case 'ESOCKET':
             case 'ECONNRESET':
-              this.pool.destroy(connectionLock);
+              this.pool.destroy(connectionLock)
+                .catch(/Resource not currently part of this pool/, () => {});
           }
         });
       }

lib/dialects/mysql/connection-manager.js

@@ -99,6 +99,8 @@ class ConnectionManager extends AbstractConnectionManager {
           debug(`connection error ${e.code}`);
 
           if (e.code === 'PROTOCOL_CONNECTION_LOST') {
+            this.pool.destroy(connection)
+              .catch(/Resource not currently part of this pool/, () => {});
             return;
           }
         }

lib/dialects/postgres/connection-manager.js

@@ -35,7 +35,6 @@ class ConnectionManager extends AbstractConnectionManager {
 
   // Expose this as a method so that the parsing may be updated when the user has added additional, custom types
   _refreshTypeParser(dataType) {
-
     if (dataType.types.postgres.oids) {
       for (const oid of dataType.types.postgres.oids) {
         this.lib.types.setTypeParser(oid, value => dataType.parse(value, oid, this.lib.types.getTypeParser));
@@ -54,7 +53,6 @@ class ConnectionManager extends AbstractConnectionManager {
   }
 
   connect(config) {
-
     config.user = config.username;
     const connectionConfig = Utils._.pick(config, [
       'user', 'password', 'host', 'database', 'port'
@@ -155,7 +153,7 @@ class ConnectionManager extends AbstractConnectionManager {
 
       return new Promise((resolve, reject) => connection.query(query, (error, result) => error ? reject(error) : resolve(result))).then(results => {
         const result = Array.isArray(results) ? results.pop() : results;
-        
+
         for (const row of result.rows) {
           let type;
           if (row.typname === 'geometry') {
@@ -174,6 +172,7 @@ class ConnectionManager extends AbstractConnectionManager {
       });
     });
   }
+
   disconnect(connection) {
     return new Promise(resolve => {
       connection.end();

lib/sequelize.js

@@ -82,12 +82,12 @@ class Sequelize {
    * @param {Object}   [options.pool] sequelize connection pool configuration
    * @param {Integer}  [options.pool.max=5] Maximum number of connection in pool
    * @param {Integer}  [options.pool.min=0] Minimum number of connection in pool
-   * @param {Integer}  [options.pool.idle=10000] The maximum time, in milliseconds, that a connection can be idle before being released
+   * @param {Integer}  [options.pool.idle=10000] The maximum time, in milliseconds, that a connection can be idle before being released. Use with combination of evict for proper working, for more details read https://github.com/coopernurse/node-pool/issues/178#issuecomment-327110870
    * @param {Integer}  [options.pool.acquire=10000] The maximum time, in milliseconds, that pool will try to get connection before throwing error
-   * @param {Integer}  [options.pool.evict=60000] The time interval, in milliseconds, for evicting stale connections. Set it to 0 to disable this feature.
+   * @param {Integer}  [options.pool.evict=10000] The time interval, in milliseconds, for evicting stale connections. Set it to 0 to disable this feature.
    * @param {Boolean}  [options.pool.handleDisconnects=true] Controls if pool should handle connection disconnect automatically without throwing errors
    * @param {Function} [options.pool.validate] A function that validates a connection. Called with client. The default function checks that client is an object, and that its state is not disconnected
-   * @param {Boolean}  [options.quoteIdentifiers=true] Set to `false` to make table names and attributes case-insensitive on Postgres and skip double quoting of them.  WARNING: Setting this to false may expose vulnerabilities and is not reccomended!
+   * @param {Boolean}  [options.quoteIdentifiers=true] Set to `false` to make table names and attributes case-insensitive on Postgres and skip double quoting of them.  WARNING: Setting this to false may expose vulnerabilities and is not recommended!
    * @param {String}   [options.transactionType='DEFERRED'] Set the default transaction type. See `Sequelize.Transaction.TYPES` for possible options. Sqlite only.
    * @param {String}   [options.isolationLevel] Set the default transaction isolation level. See `Sequelize.Transaction.ISOLATION_LEVELS` for possible options.
    * @param {Object}   [options.retry] Set of flags that control when a query is automatically retried.
@@ -315,7 +315,7 @@ class Sequelize {
    *
    * sequelize.models.modelName // The model will now be available in models under the name given to define
    */
-  define(modelName, attributes, options) { // testhint options:none
+  define(modelName, attributes, options) {
     options = options || {};
 
     options.modelName = modelName;
@@ -325,7 +325,6 @@ class Sequelize {
 
     model.init(attributes, options);
 
-
     return model;
   }
 
@@ -377,7 +376,7 @@ class Sequelize {
     if (!this.importCache[path]) {
       let defineCall = arguments.length > 1 ? arguments[1] : require(path);
       if (typeof defineCall === 'object') {
-        // ES6 module compatability
+        // ES6 module compatibility
         defineCall = defineCall.default;
       }
       this.importCache[path] = defineCall(this, DataTypes);
@@ -655,8 +654,8 @@ class Sequelize {
    * @param {RegExp} [options.match] Match a regex against the database name before syncing, a safety check for cases where force: true is used in tests but not live code
    * @param {Boolean|function} [options.logging=console.log] A function that logs sql queries, or false for no logging
    * @param {String} [options.schema='public'] The schema that the tables should be created in. This can be overriden for each table in sequelize.define
-   * @param  {String} [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only)
-   * @param {Boolean} [options.hooks=true] If hooks is true then beforeSync, afterSync, beforBulkSync, afterBulkSync hooks will be called
+   * @param {String} [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only)
+   * @param {Boolean} [options.hooks=true] If hooks is true then beforeSync, afterSync, beforeBulkSync, afterBulkSync hooks will be called
    * @param {Boolean} [options.alter=false] Alters tables to fit models. Not recommended for production use. Deletes data in columns that were removed or had their type changed in the model.
    * @return {Promise}
    */
@@ -950,7 +949,6 @@ class Sequelize {
       autoCallback = options;
       options = undefined;
     }
-    // testhint argsConform.end
 
     const transaction = new Transaction(this, options);
 
@@ -1061,9 +1059,11 @@ class Sequelize {
    *
    * Normally this is done on process exit, so you only need to call this method if you are creating multiple instances, and want
    * to garbage collect some of them.
+   *
+   * @return {Promise}
    */
   close() {
-    this.connectionManager.close();
+    return this.connectionManager.close();
   }
 
   normalizeDataType(Type) {

test/config/.docker.env

@@ -5,3 +5,7 @@ SEQ_MYSQL_PW=sequelize_test
 SEQ_PG_PORT=8998
 SEQ_PG_USER=sequelize_test
 SEQ_PG_PW=sequelize_test
+SEQ_MSSQL_PORT=8997
+SEQ_MSSQL_DB=master
+SEQ_MSSQL_USER=sa
+SEQ_MSSQL_PW=yourStrong(!)Password

test/config/config.js

@@ -26,7 +26,7 @@ module.exports = {
     database: process.env.SEQ_MSSQL_DB   || process.env.SEQ_DB   || 'sequelize_test',
     username: process.env.SEQ_MSSQL_USER || process.env.SEQ_USER || 'sequelize',
     password: process.env.SEQ_MSSQL_PW   || process.env.SEQ_PW   || 'nEGkLma26gXVHFUAHJxcmsrK',
-    host:     process.env.SEQ_MSSQL_HOST || process.env.SEQ_HOST || 'mssql.sequelizejs.com',
+    host:     process.env.SEQ_MSSQL_HOST || process.env.SEQ_HOST || '127.0.0.1',
     port:     process.env.SEQ_MSSQL_PORT || process.env.SEQ_PORT || 1433,
     dialectOptions: {
       // big insert queries need a while

test/integration/dialects/abstract/connection-manager.test.js

@@ -18,7 +18,6 @@ const poolEntry = {
 };
 
 describe('Connection Manager', () => {
-
   let sandbox;
 
   beforeEach(() => {
@@ -162,7 +161,6 @@ describe('Connection Manager', () => {
       expect(poolDrainSpy.calledOnce).to.be.true;
       expect(poolClearSpy.calledOnce).to.be.true;
     });
-
   });
 
 });

test/integration/dialects/mssql/connection-manager.test.js

@@ -56,6 +56,30 @@ if (dialect.match(/^mssql/)) {
         });
     });
 
+    it('should not throw when non pooled connection is unexpectedly closed', () => {
+      const sequelize = Support.createSequelizeInstance({ pool: { min: 1, max: 1, idle: 5000 } });
+      const cm = sequelize.connectionManager;
+
+      let conn;
+
+      return sequelize
+        .sync()
+        .then(() => cm.getConnection())
+        .then(connection => {
+          conn = connection;
+
+          // remove from pool
+          return cm.pool.destroy(connection);
+        })
+        .then(() => {
+          // unexpected disconnect
+          const unwrapConn = conn.unwrap();
+          unwrapConn.emit('error', {
+            code: 'ESOCKET'
+          });
+        });
+    });
+
     describe('Errors', () => {
       it('ECONNREFUSED', () => {
         const sequelize = Support.createSequelizeInstance({ port: 34237 });

test/integration/dialects/mysql/connector-manager.test.js

@@ -34,18 +34,20 @@ if (dialect === 'mysql') {
 
     it('accepts new queries after shutting down a connection', () => {
       // Create a sequelize instance with fast disconnecting connection
-      const sequelize = Support.createSequelizeInstance({ pool: { idle: 50, max: 1 }});
+      const sequelize = Support.createSequelizeInstance({ pool: { idle: 50, max: 1, evict: 10 }});
       const User = sequelize.define('User', { username: DataTypes.STRING });
 
       return User
         .sync({force: true})
-        .then(() => User.create({username: 'user1'}))
+        .then(() => User.create({ username: 'user1' }))
         .then(() => sequelize.Promise.delay(100))
         .then(() => {
-        // This query will be queued just after the `client.end` is executed and before its callback is called
+          expect(sequelize.connectionManager.pool.size).to.equal(0);
+          //This query will be queued just after the `client.end` is executed and before its callback is called
           return sequelize.query('SELECT COUNT(*) AS count FROM Users', { type: sequelize.QueryTypes.SELECT });
         })
         .then(count => {
+          expect(sequelize.connectionManager.pool.size).to.equal(1);
           expect(count[0].count).to.equal(1);
         });
     });
@@ -68,7 +70,7 @@ if (dialect === 'mysql') {
           return cm.getConnection();
         })
         .then(connection => {
-          // Old threadId should be different from current new one
+          // Old threadId should be same as current connection
           expect(conn.threadId).to.be.equal(connection.threadId);
           expect(cm.validate(conn)).to.be.ok;
 
@@ -77,7 +79,7 @@ if (dialect === 'mysql') {
     });
 
     it('should work with handleDisconnects before release', () => {
-      const sequelize = Support.createSequelizeInstance({pool: {min: 1, max: 1, handleDisconnects: true, idle: 5000}});
+      const sequelize = Support.createSequelizeInstance({pool: { max: 1, min: 1, handleDisconnects: true, idle: 5000 }});
       const cm = sequelize.connectionManager;
       let conn;
 
@@ -89,8 +91,9 @@ if (dialect === 'mysql') {
           conn = connection;
           // simulate a unexpected end from MySQL2
           conn.stream.emit('end');
+
+          return cm.releaseConnection(connection);
         })
-        .then(() => cm.releaseConnection(conn))
         .then(() => {
           // Get next available connection
           return cm.getConnection();
@@ -98,7 +101,9 @@ if (dialect === 'mysql') {
         .then(connection => {
           // Old threadId should be different from current new one
           expect(conn.threadId).to.not.be.equal(connection.threadId);
-          expect(cm.validate(conn)).to.not.be.ok;
+          expect(sequelize.connectionManager.pool.size).to.equal(1);
+          expect(cm.validate(conn)).to.be.not.ok;
+
           return cm.releaseConnection(connection);
         });
     });

test/integration/replication.test.js

@@ -57,26 +57,22 @@ describe(Support.getTestDialectTeaser('Replication'), function() {
   it('should be able to make a write', () => {
     return this.User.create({
       firstName: Math.random().toString()
-    })
-      .then(expectWriteCalls);
+    }).then(expectWriteCalls);
   });
 
   it('should be able to make a read', () => {
-    return this.User.findAll()
-      .then(expectReadCalls);
+    return this.User.findAll().then(expectReadCalls);
   });
 
   it('should run read-only transactions on the replica', () => {
     return this.sequelize.transaction({readOnly: true}, transaction => {
       return this.User.findAll({transaction});
-    })
-      .then(expectReadCalls);
+    }).then(expectReadCalls);
   });
 
   it('should run non-read-only transactions on the primary', () => {
     return this.sequelize.transaction(transaction => {
       return this.User.findAll({transaction});
-    })
-      .then(expectWriteCalls);
+    }).then(expectWriteCalls);
   });
 });