From ae1245ebcb7c271c0cae6a03e9ca818d94843ea7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mikael=20Lepist=C3=B6?= Date: Sun, 10 Jun 2018 23:56:08 +0300 Subject: [PATCH] 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 | 2 +- scripts/stress-test/docker-compose.yml | 17 +-- scripts/stress-test/knex-stress-test.js | 34 +++++- src/dialects/mssql/index.js | 135 +++++++++++++++++++++++- 4 files changed, 173 insertions(+), 15 deletions(-) diff --git a/package.json b/package.json index a25a089bb2..4122b17784 100644 --- a/package.json +++ b/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": { diff --git a/scripts/stress-test/docker-compose.yml b/scripts/stress-test/docker-compose.yml index 97609819b8..274e72d357 100644 --- a/scripts/stress-test/docker-compose.yml +++ b/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 diff --git a/scripts/stress-test/knex-stress-test.js b/scripts/stress-test/knex-stress-test.js index 4494514ae1..b1be938e44 100644 --- a/scripts/stress-test/knex-stress-test.js +++ b/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,19 +148,26 @@ 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 @@ -150,7 +175,8 @@ async function main() { await Promise.all([ killConnectionsPg(), killConnectionsMyslq(mysql), - killConnectionsMyslq(mysql2), +// killConnectionsMyslq(mysql2), + killConnectionsMssql() ]); } catch (err) { console.log('KILLER ERROR:', err); diff --git a/src/dialects/mssql/index.js b/src/dialects/mssql/index.js index aecc66f5ae..53b676ddb6 100644 --- a/src/dialects/mssql/index.js +++ b/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.