Fix mssql driver crashing (#2637)

* Run SQL Server tests locally running SQL server in docker

* WIP mssql test stuff

* Patched MSSQL driver to not crash knex anymore

package.json

@@ -84,7 +84,7 @@
     "mssql:test": "DB=mssql npm test",
     "mssql:destroy": "docker-compose -f scripts/mssql-docker-compose.yml stop",
     "stress:init": "docker-compose -f scripts/stress-test/docker-compose.yml up --no-start && docker-compose -f scripts/stress-test/docker-compose.yml start",
-    "stress:test": "node scripts/stress-test/knex-stress-test.js |  grep -A 3 -- '- STATS '",
+    "stress:test": "node scripts/stress-test/knex-stress-test.js |  grep -A 5 -B 60 -- '- STATS '",
     "stress:destroy": "docker-compose -f scripts/stress-test/docker-compose.yml stop"
   },
   "bin": {

scripts/stress-test/docker-compose.yml

@@ -8,9 +8,12 @@ services:
       - "23306:23306"
       - "25432:25432"
       - "21521:21521"
+      - "21433:21433"
     links:
       - "mysql"
       - "postgresql"
+      - "oracledbxe"
+      - "mssql"
 
   mysql:
     image: mysql:5.7
@@ -35,10 +38,10 @@ services:
     environment:
       - ORACLE_ALLOW_REMOTE=true
 
-#  mssql:
-#    image: microsoft/mssql-server-linux
-#    ports:
-#      - "31433:1433"
-#    environment:
-#      - ACCEPT_EULA=Y
-#      - SA_PASSWORD=mssqlpassword
+  mssql:
+    image: microsoft/mssql-server-linux:2017-latest
+    ports:
+      - "31433:1433"
+    environment:
+      - ACCEPT_EULA=Y
+      - SA_PASSWORD=S0meVeryHardPassword

scripts/stress-test/knex-stress-test.js

@@ -24,6 +24,18 @@ const mysql = Knex({
   pool: { max: 50 }
 });
 
+const mssql = Knex({
+  client: 'mssql',
+  connection: {
+    port: 21433,
+    user: "sa",
+    password: "S0meVeryHardPassword",
+    server: "localhost",
+    requestTimeout: 500
+  },
+  pool: { max: 50 }
+});
+
 /* TODO: figure out how to nicely install oracledb node driver on osx
 const mysql = Knex({
   client: 'oracledb',
@@ -50,6 +62,7 @@ function setQueryCounters(instance, name) {
 setQueryCounters(pg, 'pg');
 setQueryCounters(mysql, 'mysql');
 setQueryCounters(mysql2, 'mysql2');
+setQueryCounters(mssql, 'mssql');
 
 const _ = require('lodash');
 
@@ -84,6 +97,11 @@ async function killConnectionsMyslq(client) {
   await Promise.all(rows.map(row => client.raw(`KILL ${row.Id}`)));
 } 
 
+async function killConnectionsMssql() {
+  const rows = await mssql('sys.dm_exec_sessions').select('session_id');
+  await Promise.all(rows.map(row => mssql.raw(`KILL ${row.session_id}`)));
+} 
+
 async function main() {
   async function loopQueries(prefix, query) {
     const queries = () => [
@@ -130,27 +148,35 @@ async function main() {
     await recreateProxy('postgresql', 25432, 5432);
     await recreateProxy('mysql', 23306, 3306);
     await recreateProxy('oracledbxe', 21521, 1521);
+    await recreateProxy('mssql', 21433, 1433);
   }
 
   await recreateProxies();
-  
+
   loopQueries('PSQL:', pg.raw('select 1'));
   loopQueries('PSQL TO:', pg.raw('select 1').timeout(20));
 
   loopQueries('MYSQL:', mysql.raw('select 1'));
   loopQueries('MYSQL TO:', mysql.raw('select 1').timeout(20));
 
-  loopQueries('MYSQL2:', mysql2.raw('select 1'));
-  loopQueries('MYSQL2 TO:', mysql2.raw('select 1').timeout(20));
+// mysql2 still crashes app (without connection killer nor timeouts)
+// https://github.com/sidorares/node-mysql2/issues/731
+//  loopQueries('MYSQL2:', mysql2.raw('select 1'));
+//  loopQueries('MYSQL2 TO:', mysql2.raw('select 1').timeout(20));
+
+  loopQueries('MSSQL:', mssql.raw('select 1'));
+  loopQueries('MSSQL TO:', mssql.raw('select 1').timeout(20));
 
+  setInterval(recreateProxies, 2000);
 
   while(true) {
     await Bluebird.delay(20); // kill everything every quite often from server side
     try {
       await Promise.all([
         killConnectionsPg(),
         killConnectionsMyslq(mysql),
-        killConnectionsMyslq(mysql2),
+//        killConnectionsMyslq(mysql2),
+        killConnectionsMssql()  
       ]);
     } catch (err) {
       console.log('KILLER ERROR:', err);

src/dialects/mssql/index.js

@@ -21,12 +21,21 @@ const SQL_BIGINT_SAFE = { MIN : -9007199254740991, MAX: 9007199254740991}
 
 // Always initialize with the "QueryBuilder" and "QueryCompiler" objects, which
 // extend the base 'lib/query/builder' and 'lib/query/compiler', respectively.
-function Client_MSSQL(config) {
+function Client_MSSQL(config = {}) {
   // #1235 mssql module wants 'server', not 'host'. This is to enforce the same
   // options object across all dialects.
   if(config && config.connection && config.connection.host) {
     config.connection.server = config.connection.host;
   }
+
+  // mssql always creates pool :( lets try to unpool it as much as possible
+  config.pool = {
+    min: 1,
+    max: 1,
+    idleTimeoutMillis: Number.MAX_SAFE_INTEGER,
+    evictionRunIntervalMillis: 0
+  };
+
   Client.call(this, config);
 }
 inherits(Client_MSSQL, Client);
@@ -38,7 +47,124 @@ assign(Client_MSSQL.prototype, {
   driverName: 'mssql',
 
   _driver() {
-    return require('mssql');
+    const tds = require('tedious');
+    const mssqlTedious = require('mssql');
+    const base = require('mssql/lib/base');
+
+    // Monkey patch mssql's tedious driver _poolCreate method to fix problem with hanging acquire
+    // connection, this should be removed when https://github.com/tediousjs/node-mssql/pull/614 is
+    // merged and released.
+
+    // Also since this dialect actually always uses tedious driver (msnodesqlv8 driver should be
+    // required in different way), it might be better to use tedious directly, because mssql
+    // driver uses always internally extra generic-pool and just adds one unnecessary layer of
+    // indirection between database and knex and mssql driver has been lately without maintainer
+    // (changing implementation to use tedious will be breaking change though).
+
+    // TODO: remove mssql implementation all together and use tedious directly
+
+    const mssqlVersion = require('mssql/package.json').version;
+    if (mssqlVersion !== '4.1.0') {
+      throw new Error(
+        'This knex version does not support any other mssql version except 4.1.0 ' +
+        '(knex patches bug in its implementation)');
+    }
+
+    mssqlTedious.ConnectionPool.prototype.release = release;
+    mssqlTedious.ConnectionPool.prototype._poolCreate = _poolCreate;
+
+    // in some rare situations release is called when stream is interrupted, but 
+    // after pool is already destroyed
+    function release(connection) {
+      if (this.pool) {
+        this.pool.release(connection);
+      }
+    }
+
+    function _poolCreate() {
+      // implementation is copy-pasted from https://github.com/tediousjs/node-mssql/pull/614
+      return new base.Promise((resolve, reject) => {
+        const cfg = {
+          userName: this.config.user,
+          password: this.config.password,
+          server: this.config.server,
+          options: Object.assign({}, this.config.options),
+          domain: this.config.domain
+        }
+
+        cfg.options.database = this.config.database
+        cfg.options.port = this.config.port
+        cfg.options.connectTimeout = this.config.connectionTimeout || this.config.timeout || 15000
+        cfg.options.requestTimeout =
+          this.config.requestTimeout != null ? this.config.requestTimeout : 15000
+        cfg.options.tdsVersion = cfg.options.tdsVersion || '7_4'
+        cfg.options.rowCollectionOnDone = false
+        cfg.options.rowCollectionOnRequestCompletion = false
+        cfg.options.useColumnNames = false
+        cfg.options.appName = cfg.options.appName || 'node-mssql'
+
+        // tedious always connect via tcp when port is specified
+        if (cfg.options.instanceName) delete cfg.options.port
+
+        if (isNaN(cfg.options.requestTimeout)) cfg.options.requestTimeout = 15000
+        if (cfg.options.requestTimeout === Infinity) cfg.options.requestTimeout = 0
+        if (cfg.options.requestTimeout < 0) cfg.options.requestTimeout = 0
+
+        if (this.config.debug) {
+          cfg.options.debug = {
+            packet: true,
+            token: true,
+            data: true,
+            payload: true
+          }
+        }
+
+        const tedious = new tds.Connection(cfg)
+
+        // prevent calling resolve again on end event
+        let alreadyResolved = false
+        function safeResolve (err) {
+          if (!alreadyResolved) {
+            alreadyResolved = true
+            resolve(err)
+          }
+        }
+
+        function safeReject (err) {
+          if (!alreadyResolved) {
+            alreadyResolved = true
+            reject(err)
+          }
+        }
+
+        tedious.once('end', evt => {
+          safeReject(new base.ConnectionError('Connection ended unexpectedly during connecting'))
+        })
+
+        tedious.once('connect', err => {
+          if (err) {
+            err = new base.ConnectionError(err)
+            return safeReject(err)
+          }
+          safeResolve(tedious)
+        })
+
+        tedious.on('error', err => {
+          if (err.code === 'ESOCKET') {
+            tedious.hasError = true
+            return
+          }
+
+          this.emit('error', err)
+        })
+
+        if (this.config.debug) {
+          tedious.on('debug', this.emit.bind(this, 'debug', tedious))
+        }
+      })
+    }
+
+    return mssqlTedious;
   },
 
   formatter() {
@@ -97,7 +223,10 @@ assign(Client_MSSQL.prototype, {
   // Used to explicitly close a connection, called internally by the pool
   // when a connection times out or the pool is shutdown.
   destroyRawConnection(connection) {
-    return connection.close()
+    return connection.close().catch(err => {
+      // some times close will reject just because pool has already been destoyed 
+      // internally by the driver there is nothing we can do in this case
+    });
   },
 
   // Position the bindings for the query.