From cba3d4ad3817e4a47349ac2065e9958da8d5b250 Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Wed, 13 Nov 2019 15:24:25 +0000 Subject: [PATCH 01/12] Allow repeat calls to ConnectionPool.connect() to resolve --- lib/base/connection-pool.js | 23 +++++++++++++++++------ test/common/tests.js | 13 ++++++++++++- test/msnodesqlv8/msnodesqlv8.js | 1 + test/tedious/tedious.js | 1 + 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/lib/base/connection-pool.js b/lib/base/connection-pool.js index eeaec6cd..5e13a67d 100644 --- a/lib/base/connection-pool.js +++ b/lib/base/connection-pool.js @@ -34,6 +34,8 @@ class ConnectionPool extends EventEmitter { IDS.add(this, 'ConnectionPool') debug('pool(%d): created', IDS.get(this)) + this._connectStack = [] + this._connected = false this._connecting = false this._healthy = false @@ -152,11 +154,13 @@ class ConnectionPool extends EventEmitter { _connect (callback) { if (this._connected) { - return setImmediate(callback, new ConnectionError('Database is already connected! Call close before connecting to different database.', 'EALREADYCONNECTED')) + return setImmediate(callback, null, this) } + this._connectStack.push(callback) + if (this._connecting) { - return setImmediate(callback, new ConnectionError('Already connecting to database! Call close before connecting to different database.', 'EALREADYCONNECTING')) + return } this._connecting = true @@ -167,8 +171,9 @@ class ConnectionPool extends EventEmitter { debug('pool(%d): connected', IDS.get(this)) this._healthy = true - this._poolDestroy(connection).then(() => { + return this._poolDestroy(connection).then(() => { if (!this._connecting) { + this._connectStack = [] debug('pool(%d): not connecting, exiting silently (was close called before connection established?)', IDS.get(this)) return } @@ -233,12 +238,18 @@ class ConnectionPool extends EventEmitter { this._connecting = false this._connected = true - - callback(null, this) + }) + }).then(() => { + this._connectStack.forEach((cb) => { + setImmediate(cb, null, this) }) }).catch(err => { this._connecting = false - callback(err) + this._connectStack.forEach((cb) => { + setImmediate(cb, err) + }) + }).then(() => { + this._connectStack = [] }) } diff --git a/test/common/tests.js b/test/common/tests.js index 3243c2be..1d1f445d 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -827,6 +827,17 @@ module.exports = (sql, driver) => { req.cancel() }, + 'repeat calls to connect resolve' (config, done) { + const pool = new sql.ConnectionPool(config) + Promise.all([pool.connect(), pool.connect()]) + .then(([pool1, pool2]) => { + assert.strictEqual(pool, pool1) + assert.strictEqual(pool, pool2) + done() + }) + .catch(done) + }, + 'connection healthy works' (config, done) { const pool = new sql.ConnectionPool(config) assert.ok(!pool.healthy) @@ -836,7 +847,7 @@ module.exports = (sql, driver) => { }).then(() => { assert.ok(!pool.healthy) done() - }) + }).catch(done) }, 'healthy connection goes bad' (config, done) { diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 4c80eab9..07fcb571 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -83,6 +83,7 @@ describe('msnodesqlv8', function () { it('transaction throws on bad isolation level', done => TESTS['transaction throws on bad isolation level'](done)) it('transaction accepts good isolation levels', done => TESTS['transaction accepts good isolation levels'](done)) it('cancel request', done => TESTS['cancel request'](done)) + it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) it('connection healthy works', done => TESTS['connection healthy works'](config(), done)) it('healthy connection goes bad', done => TESTS['healthy connection goes bad'](config(), done)) it('request timeout', done => TESTS['request timeout'](done)) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index 4382bfe1..c79071b6 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -82,6 +82,7 @@ describe('tedious', () => { it('transaction with error (XACT_ABORT set to ON)', done => TESTS['transaction with error'](done)) it('transaction with synchronous error', done => TESTS['transaction with synchronous error'](done)) it('cancel request', done => TESTS['cancel request'](done, /Canceled./)) + it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) it('connection healthy works', done => TESTS['connection healthy works'](config(), done)) it('healthy connection goes bad', done => TESTS['healthy connection goes bad'](config(), done)) it('request timeout', done => TESTS['request timeout'](done, 'tedious', /Timeout: Request failed to complete in 1000ms/)) From 5d37889d39b65c69bff04e359bb270b00a239a0b Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Thu, 14 Nov 2019 13:08:48 +0000 Subject: [PATCH 02/12] Allow repeat calls to ConnectionPool.close() --- lib/base/connection-pool.js | 23 ++++++++++++++++++++--- test/common/tests.js | 20 ++++++++++++++++---- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/lib/base/connection-pool.js b/lib/base/connection-pool.js index 5e13a67d..0eec3f6e 100644 --- a/lib/base/connection-pool.js +++ b/lib/base/connection-pool.js @@ -35,6 +35,7 @@ class ConnectionPool extends EventEmitter { debug('pool(%d): created', IDS.get(this)) this._connectStack = [] + this._closeStack = [] this._connected = false this._connecting = false @@ -105,6 +106,8 @@ class ConnectionPool extends EventEmitter { _acquire () { if (!this.pool) { return shared.Promise.reject(new ConnectionError('Connection not yet open.', 'ENOTOPEN')) + } else if (this.pool.destroyed) { + return shared.Promise.reject(new ConnectionError('Connection is closing', 'ENOTOPEN')) } return this.pool.acquire() @@ -300,9 +303,23 @@ class ConnectionPool extends EventEmitter { if (!this.pool) return setImmediate(callback, null) - this.pool.destroy() - this.pool = null - callback(null) + this._closeStack.push(callback) + + if (this.pool.destroyed) return + + this.pool.destroy().then(() => { + this.pool = null + this._closeStack.forEach(cb => { + setImmediate(cb, null) + }) + }).catch(err => { + this.pool = null + this._closeStack.forEach(cb => { + setImmediate(cb, err) + }) + }).then(() => { + this._closeStack = [] + }) } /** diff --git a/test/common/tests.js b/test/common/tests.js index 1d1f445d..bac24552 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -769,6 +769,10 @@ module.exports = (sql, driver) => { const tran = new TestTransaction() return tran.begin(ISOLATION_LEVELS[level]).then(() => { return tran.request().query('SELECT 1 AS num') + .catch(err => { + return tran.abort().then(() => Promise.reject(err)) + }) + .then(() => tran.commit()) }) }) Promise.all(promises).then(() => done()).catch(err => { @@ -864,16 +868,24 @@ module.exports = (sql, driver) => { pool._poolCreate = () => { return Promise.reject(new sql.ConnectionError('Synthetic error')) } - return pool.acquire().then(() => { - assert.fail('acquire resolved unexpectedly') + return pool.acquire().then((conn) => { + try { + assert.fail('acquire resolved unexpectedly') + } finally { + pool.release(conn) + } }).catch(() => { assert.ok(pool.pool.numUsed() + pool.pool.numFree() <= 0) assert.ok(!pool.healthy) }) }).then(() => { pool._poolCreate = ogCreate - return pool.acquire().then(() => { - assert.ok(pool.healthy) + return pool.acquire().then((conn) => { + try { + assert.ok(pool.healthy) + } finally { + pool.release(conn) + } }) }).then(() => { return pool.close() From 481f789c6aead0d9cd96173c428cf6d35174796d Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Fri, 15 Nov 2019 11:41:33 +0000 Subject: [PATCH 03/12] Fix an issue where paused msnodesqlv8 connections dont emit a done event on cancel --- lib/msnodesqlv8/connection-pool.js | 2 +- lib/msnodesqlv8/request.js | 6 ++++++ test/common/tests.js | 26 ++++++++++++++++++++++++++ test/msnodesqlv8/msnodesqlv8.js | 6 +++--- test/tedious/tedious.js | 6 +++--- 5 files changed, 39 insertions(+), 7 deletions(-) diff --git a/lib/msnodesqlv8/connection-pool.js b/lib/msnodesqlv8/connection-pool.js index 49662306..a53b575e 100644 --- a/lib/msnodesqlv8/connection-pool.js +++ b/lib/msnodesqlv8/connection-pool.js @@ -76,9 +76,9 @@ class ConnectionPool extends BaseConnectionPool { } debug('connection(%d): destroying', IDS.get(tds)) tds.close(() => { + debug('connection(%d): destroyed', IDS.get(tds)) resolve() }) - debug('connection(%d): destroyed', IDS.get(tds)) }) } } diff --git a/lib/msnodesqlv8/request.js b/lib/msnodesqlv8/request.js index 03e8126e..c2a6e9d0 100644 --- a/lib/msnodesqlv8/request.js +++ b/lib/msnodesqlv8/request.js @@ -321,6 +321,12 @@ class Request extends BaseRequest { debug('request(%d): cancel', IDS.get(this)) req.cancelQuery(err => { if (err) debug('request(%d): failed to cancel', IDS.get(this), err) + // this fixes an issue where paused connections don't emit a done event + try { + if (req.isPaused()) req.emit('done') + } catch (err) { + // do nothing + } }) } diff --git a/test/common/tests.js b/test/common/tests.js index bac24552..4af2e9d4 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -1361,6 +1361,32 @@ module.exports = (sql, driver) => { }) }, + 'a cancelled paused stream emits done event' (done) { + let rows = 0 + + const req = new TestRequest() + req.stream = true + req.query('select * from streaming') + req.on('error', (err) => { + if (err.code !== 'ECANCEL') { + req.cancel() + done(err) + } + }) + + req.on('done', () => { + done() + }) + + req.on('row', row => { + rows++ + if (rows >= 10) { + req.pause() + req.cancel() + } + }) + }, + 'streaming resume' (done) { let rows = 0 let started = false diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 07fcb571..966a3fe2 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -199,11 +199,10 @@ describe('msnodesqlv8', function () { }) describe('msnodesqlv8 stress', function () { - this.timeout(600000) before((done) => { const cfg = config() cfg.options.abortTransactionOnError = true - cfg.requestTimeout = 60000 + // cfg.requestTimeout = 60000 sql.connect(cfg, done) }) @@ -212,8 +211,9 @@ describe('msnodesqlv8', function () { it('streaming off', done => TESTS['streaming off'](done)) it('streaming on', done => TESTS['streaming on'](done)) itNode10('streaming pause', done => TESTS['streaming pause'](done)) - itNode10('a cancelled stream emits done event', done => TESTS['a cancelled stream emits done event'](done)) itNode10('streaming resume', done => TESTS['streaming resume'](done)) + itNode10('a cancelled stream emits done event', done => TESTS['a cancelled stream emits done event'](done)) + itNode10('a cancelled paused stream emits done event', done => TESTS['a cancelled paused stream emits done event'](done)) after(done => sql.close(done)) }) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index c79071b6..a67508bd 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -240,11 +240,10 @@ describe('tedious', () => { }) describe('Stress', function stress () { - this.timeout(600000) before((done) => { const cfg = config() cfg.options.abortTransactionOnError = true - cfg.requestTimeout = 60000 + // cfg.requestTimeout = 60000 sql.connect(cfg, done) }) @@ -253,8 +252,9 @@ describe('tedious', () => { it('streaming off', done => TESTS['streaming off'](done)) it('streaming on', done => TESTS['streaming on'](done)) it('streaming pause', done => TESTS['streaming pause'](done)) - it('a cancelled stream emits done event', done => TESTS['a cancelled stream emits done event'](done)) it('streaming resume', done => TESTS['streaming resume'](done)) + it('a cancelled stream emits done event', done => TESTS['a cancelled stream emits done event'](done)) + it('a cancelled paused stream emits done event', done => TESTS['a cancelled paused stream emits done event'](done)) after(done => sql.close(done)) }) From fc53f8694ad3bad09b76128aff82a3ac7363c79d Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Fri, 15 Nov 2019 12:09:43 +0000 Subject: [PATCH 04/12] Dont emit final row events if stream is paused --- lib/base/request.js | 4 ++++ lib/msnodesqlv8/request.js | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/base/request.js b/lib/base/request.js index 1509efb2..4c0f2f8d 100644 --- a/lib/base/request.js +++ b/lib/base/request.js @@ -40,6 +40,10 @@ class Request extends EventEmitter { this.parameters = {} } + get paused () { + return this._paused + } + /** * Generate sql string and set imput parameters from tagged template string. * diff --git a/lib/msnodesqlv8/request.js b/lib/msnodesqlv8/request.js index c2a6e9d0..90d7cf73 100644 --- a/lib/msnodesqlv8/request.js +++ b/lib/msnodesqlv8/request.js @@ -361,7 +361,7 @@ class Request extends BaseRequest { if (row && row.___return___ == null) { // row with ___return___ col is the last row - if (this.stream) this.emit('row', row) + if (this.stream && !this.paused) this.emit('row', row) } } @@ -402,7 +402,7 @@ class Request extends BaseRequest { if (row.___return___ == null) { // row with ___return___ col is the last row - if (this.stream) this.emit('row', row) + if (this.stream && !this.paused) this.emit('row', row) } } @@ -509,7 +509,7 @@ class Request extends BaseRequest { if (row && row.___return___ == null) { // row with ___return___ col is the last row - if (this.stream) { this.emit('row', row) } + if (this.stream && !this.paused) { this.emit('row', row) } } } From 9ea91d0aaf2f279f7608939fb62819c5c322b77d Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Sat, 16 Nov 2019 12:59:57 +0000 Subject: [PATCH 05/12] Add docs for ConnectionPool --- README.md | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/README.md b/README.md index 5e6305ba..d4d72130 100644 --- a/README.md +++ b/README.md @@ -480,6 +480,14 @@ function runStoredProcedure() { Awaiting or `.then`ing the pool creation is a safe way to ensure that the pool is always ready, without knowing where it is needed first. In practice, once the pool is created then there will be no delay for the next operation. +As of v6.1.0 you can make repeat calls to `ConnectionPool.connect()` and `ConnectonPool.close()` without an error being +thrown, allowing for the safe use of `mssql.connect().then(...)` throughout your code as well as making multiple calls to +close when your application is shutting down. + +The ability to call `connect()` repeatedly is intended to make pool management easier, however it is still recommended +to follow the example above where `connect()` is called once and using the original resolved connection promise. +Repeatedly calling `connect()` when running queries risks running into problems when `close()` is called on the pool. + **ES6 Tagged template literals** ```javascript @@ -494,6 +502,83 @@ new sql.ConnectionPool(config).connect().then(pool => { All values are automatically sanitized against sql injection. +### Managing connection pools + +Most applications will only need a single `ConnectionPool` that can be shared throughout the code. To aid the sharing +of a single pool this library exposes a set of functions to access a single global connection. eg: + +```js +// as part of your application's boot process + +const sql = require('mssql') +const poolPromise = sql.connect() + +// during your applications runtime + +poolPromise.then(() => { + return sql.query('SELECT 1') +}).then(result => { + console.dir(result) +}) + +// when your application exits +poolPromise.then(() => { + return sql.close() +}) +``` + +If you require multiple pools per application (perhaps you have many DBs you need to connect to or you want a read-only +pool), then you will need to manage your pools yourself. The best way to do this is to create a shared library file that +can hold references to the pools for you. For example: + +```js +const sql = require('mssql') + +const pools = {} + +// manage a set of pools by name (config will be required to create the pool) +// a pool will be removed when it is closed +async function getPool(name, config) { + if (!Object.prototype.hasOwnProperty.call(pools, name)) { + const pool = new sql.ConnectionPool(config) + const close = pool.close.bind(pool) + pool.close = (...args) => { + delete pools[name] + return close(...args) + } + await pool.connect() + pools[name] = pool + } + return pools[name] +} + +// close all pools +function closeAll() { + return Promise.all(Object.values(pools).map((pool) => { + return pool.close() + })) +} + +module.exports = { + closeAll, + getPool +} +``` + +You can then use this library file in your code to get a connected pool when you need it: + +```js +const { getPool } = require('./path/to/file') + +// run a query +async function runQuery(query, config) { + // pool will always be connected when the promise has resolved - may reject if the connection config is invalid + const pool = await getPool('default', config) + const result = await pool.request().query(query) + return result +} +``` + ## Configuration ```javascript From aeb2ff50f3429919fb40eb48aee356c7583ecf45 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Sat, 16 Nov 2019 13:00:14 +0000 Subject: [PATCH 06/12] Add some debug calls and comments --- lib/base/connection-pool.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/base/connection-pool.js b/lib/base/connection-pool.js index 0eec3f6e..c5b0ea11 100644 --- a/lib/base/connection-pool.js +++ b/lib/base/connection-pool.js @@ -175,6 +175,9 @@ class ConnectionPool extends EventEmitter { this._healthy = true return this._poolDestroy(connection).then(() => { + // this feels risky because it could lead to permanently unresolved promises / callbacks + // just because one thread has called connect and another has called close shouldn't mean + // the promises never resolve, it feels like it should either error or reconnect if (!this._connecting) { this._connectStack = [] debug('pool(%d): not connecting, exiting silently (was close called before connection established?)', IDS.get(this)) @@ -301,13 +304,17 @@ class ConnectionPool extends EventEmitter { _close (callback) { this._connecting = this._connected = this._healthy = false - if (!this.pool) return setImmediate(callback, null) + if (!this.pool) { + debug('pool(%d): already closed, executing close callback immediately', IDS.get(this)) + return setImmediate(callback, null) + } this._closeStack.push(callback) if (this.pool.destroyed) return this.pool.destroy().then(() => { + debug('pool(%d): pool closed, removing pool reference and executing close callbacks', IDS.get(this)) this.pool = null this._closeStack.forEach(cb => { setImmediate(cb, null) From 60817fdb9f7157ee721e4d97508ed3381823952d Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Sat, 16 Nov 2019 13:00:52 +0000 Subject: [PATCH 07/12] Allow a pool to reconnect if connect is called whilst close is executing --- lib/base/connection-pool.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/base/connection-pool.js b/lib/base/connection-pool.js index c5b0ea11..95fa51ff 100644 --- a/lib/base/connection-pool.js +++ b/lib/base/connection-pool.js @@ -157,9 +157,24 @@ class ConnectionPool extends EventEmitter { _connect (callback) { if (this._connected) { + debug('pool(%d): already connected, executing connect callback immediately', IDS.get(this)) return setImmediate(callback, null, this) } + // if the pool is not connected but a pool exists then connect was + // called whilst the pool is being closed + if (this.pool && this.pool.destroyed) { + debug('pool(%d): pool connect called when pool is closing, queuing connect', IDS.get(this)) + this.close(err => { + if (err) { + setImmediate(callback, err) + } else { + this._connect(callback) + } + }) + return + } + this._connectStack.push(callback) if (this._connecting) { From fbe85a0bc13800b53aae2a47f9772bae8fb2e678 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Sat, 16 Nov 2019 13:22:42 +0000 Subject: [PATCH 08/12] Add tests for calls to connect before close has resolved --- test/common/tests.js | 17 +++++++++++++++++ test/msnodesqlv8/msnodesqlv8.js | 1 + test/tedious/tedious.js | 1 + 3 files changed, 19 insertions(+) diff --git a/test/common/tests.js b/test/common/tests.js index 4af2e9d4..44f027e2 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -842,6 +842,23 @@ module.exports = (sql, driver) => { .catch(done) }, + 'calls to connect then close then connect resolve' (config, done) { + const pool = new sql.ConnectionPool(config) + let poolClosed = false + // these will run in parallel + pool.connect().catch(done) + pool.close().then(() => { poolClosed = true }).catch(done) + assert.ok(!pool.connecting) + assert.ok(!pool.connected) + // this will be called whilst + pool.connect().then(connected => { + assert.ok(poolClosed) + return connected.query('SELECT 1') + }).then(() => { + done() + }).catch(done) + }, + 'connection healthy works' (config, done) { const pool = new sql.ConnectionPool(config) assert.ok(!pool.healthy) diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 966a3fe2..2c40a401 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -84,6 +84,7 @@ describe('msnodesqlv8', function () { it('transaction accepts good isolation levels', done => TESTS['transaction accepts good isolation levels'](done)) it('cancel request', done => TESTS['cancel request'](done)) it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) + it('calls to connect then close then connect resolve', done => TESTS['calls to connect then close then connect resolve'](config(), done)) it('connection healthy works', done => TESTS['connection healthy works'](config(), done)) it('healthy connection goes bad', done => TESTS['healthy connection goes bad'](config(), done)) it('request timeout', done => TESTS['request timeout'](done)) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index a67508bd..d41f1413 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -83,6 +83,7 @@ describe('tedious', () => { it('transaction with synchronous error', done => TESTS['transaction with synchronous error'](done)) it('cancel request', done => TESTS['cancel request'](done, /Canceled./)) it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) + it('calls to connect then close then connect resolve', done => TESTS['calls to connect then close then connect resolve'](config(), done)) it('connection healthy works', done => TESTS['connection healthy works'](config(), done)) it('healthy connection goes bad', done => TESTS['healthy connection goes bad'](config(), done)) it('request timeout', done => TESTS['request timeout'](done, 'tedious', /Timeout: Request failed to complete in 1000ms/)) From 0b1843170a10c0a8c9220e3c3176e7cd04c4dddc Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Sun, 17 Nov 2019 23:34:26 +0000 Subject: [PATCH 09/12] Dont allow pool to get into a state where it can be closde during connecting phase --- lib/base/connection-pool.js | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/base/connection-pool.js b/lib/base/connection-pool.js index 95fa51ff..1cf32910 100644 --- a/lib/base/connection-pool.js +++ b/lib/base/connection-pool.js @@ -190,15 +190,6 @@ class ConnectionPool extends EventEmitter { this._healthy = true return this._poolDestroy(connection).then(() => { - // this feels risky because it could lead to permanently unresolved promises / callbacks - // just because one thread has called connect and another has called close shouldn't mean - // the promises never resolve, it feels like it should either error or reconnect - if (!this._connecting) { - this._connectStack = [] - debug('pool(%d): not connecting, exiting silently (was close called before connection established?)', IDS.get(this)) - return - } - // prepare pool this.pool = new tarn.Pool( Object.assign({ @@ -317,7 +308,17 @@ class ConnectionPool extends EventEmitter { */ _close (callback) { - this._connecting = this._connected = this._healthy = false + if (this._connecting) { + debug('pool(%d): pool close called when pool is connecting, queuing close', IDS.get(this)) + this.connect(err => { + if (err) { + setImmediate(callback, err) + } else { + this._close(callback) + } + }) + return + } if (!this.pool) { debug('pool(%d): already closed, executing close callback immediately', IDS.get(this)) @@ -328,6 +329,8 @@ class ConnectionPool extends EventEmitter { if (this.pool.destroyed) return + this._connecting = this._connected = this._healthy = false + this.pool.destroy().then(() => { debug('pool(%d): pool closed, removing pool reference and executing close callbacks', IDS.get(this)) this.pool = null From a730aa18f7b31d99dd4ddb44e508bbec099b2a37 Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Thu, 30 Jan 2020 21:22:24 +0000 Subject: [PATCH 10/12] Dont allow a pool to be closed when it is connecting --- lib/base/connection-pool.js | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/lib/base/connection-pool.js b/lib/base/connection-pool.js index 1cf32910..102f56fd 100644 --- a/lib/base/connection-pool.js +++ b/lib/base/connection-pool.js @@ -161,20 +161,6 @@ class ConnectionPool extends EventEmitter { return setImmediate(callback, null, this) } - // if the pool is not connected but a pool exists then connect was - // called whilst the pool is being closed - if (this.pool && this.pool.destroyed) { - debug('pool(%d): pool connect called when pool is closing, queuing connect', IDS.get(this)) - this.close(err => { - if (err) { - setImmediate(callback, err) - } else { - this._connect(callback) - } - }) - return - } - this._connectStack.push(callback) if (this._connecting) { @@ -308,16 +294,11 @@ class ConnectionPool extends EventEmitter { */ _close (callback) { + // we don't allow pools in a connecting state to be closed because it means there are far too many + // edge cases to deal with if (this._connecting) { - debug('pool(%d): pool close called when pool is connecting, queuing close', IDS.get(this)) - this.connect(err => { - if (err) { - setImmediate(callback, err) - } else { - this._close(callback) - } - }) - return + debug('pool(%d): close called while connecting', IDS.get(this)) + setImmediate(callback, new ConnectionError('Cannot close a pool while it is connecting')) } if (!this.pool) { From e8d52be66c11b87aef1c2b16abb3be784e531014 Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Fri, 31 Jan 2020 07:49:39 +0000 Subject: [PATCH 11/12] Replace bad test with assertion close during connect throws --- test/common/tests.js | 20 ++++++++------------ test/msnodesqlv8/msnodesqlv8.js | 2 +- test/tedious/tedious.js | 2 +- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/test/common/tests.js b/test/common/tests.js index 44f027e2..3be2d387 100644 --- a/test/common/tests.js +++ b/test/common/tests.js @@ -842,19 +842,15 @@ module.exports = (sql, driver) => { .catch(done) }, - 'calls to connect then close then connect resolve' (config, done) { + 'calls to close during connection throw' (config, done) { const pool = new sql.ConnectionPool(config) - let poolClosed = false - // these will run in parallel - pool.connect().catch(done) - pool.close().then(() => { poolClosed = true }).catch(done) - assert.ok(!pool.connecting) - assert.ok(!pool.connected) - // this will be called whilst - pool.connect().then(connected => { - assert.ok(poolClosed) - return connected.query('SELECT 1') - }).then(() => { + Promise.all([ + pool.connect(), + pool.close().catch((err) => { + assert.strictEqual(err.message, 'Cannot close a pool while it is connecting') + }) + ]).then(() => { + assert.ok(pool.connected) done() }).catch(done) }, diff --git a/test/msnodesqlv8/msnodesqlv8.js b/test/msnodesqlv8/msnodesqlv8.js index 2c40a401..efc3a43d 100644 --- a/test/msnodesqlv8/msnodesqlv8.js +++ b/test/msnodesqlv8/msnodesqlv8.js @@ -84,7 +84,7 @@ describe('msnodesqlv8', function () { it('transaction accepts good isolation levels', done => TESTS['transaction accepts good isolation levels'](done)) it('cancel request', done => TESTS['cancel request'](done)) it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) - it('calls to connect then close then connect resolve', done => TESTS['calls to connect then close then connect resolve'](config(), done)) + it('calls to close during connection throw', done => TESTS['calls to close during connection throw'](config(), done)) it('connection healthy works', done => TESTS['connection healthy works'](config(), done)) it('healthy connection goes bad', done => TESTS['healthy connection goes bad'](config(), done)) it('request timeout', done => TESTS['request timeout'](done)) diff --git a/test/tedious/tedious.js b/test/tedious/tedious.js index d41f1413..0c081772 100644 --- a/test/tedious/tedious.js +++ b/test/tedious/tedious.js @@ -83,7 +83,7 @@ describe('tedious', () => { it('transaction with synchronous error', done => TESTS['transaction with synchronous error'](done)) it('cancel request', done => TESTS['cancel request'](done, /Canceled./)) it('allows repeat calls to connect', done => TESTS['repeat calls to connect resolve'](config(), done)) - it('calls to connect then close then connect resolve', done => TESTS['calls to connect then close then connect resolve'](config(), done)) + it('calls to close during connection throw', done => TESTS['calls to close during connection throw'](config(), done)) it('connection healthy works', done => TESTS['connection healthy works'](config(), done)) it('healthy connection goes bad', done => TESTS['healthy connection goes bad'](config(), done)) it('request timeout', done => TESTS['request timeout'](done, 'tedious', /Timeout: Request failed to complete in 1000ms/)) From 4d3a7a8912c42a3223ecdfcd646976682ec99d8b Mon Sep 17 00:00:00 2001 From: Dan Hensby Date: Fri, 31 Jan 2020 17:50:42 +0000 Subject: [PATCH 12/12] Simplify global.connect() --- lib/global-connection.js | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/lib/global-connection.js b/lib/global-connection.js index ee220f60..512c00e2 100644 --- a/lib/global-connection.js +++ b/lib/global-connection.js @@ -4,7 +4,6 @@ const shared = require('./shared') let globalConnection = null const globalConnectionHandlers = {} -let onGlobalConnect = [] /** * Open global connection pool. @@ -49,35 +48,7 @@ function connect (config, callback) { globalConnection.close = globalClose.bind(globalConnection) } - if (!globalConnection.connected && !globalConnection.connecting) { - globalConnection.connect((err, pool) => { - onGlobalConnect.forEach(cb => { - setImmediate(cb, err, pool) - }) - onGlobalConnect = [] - }) - } - if (globalConnection.connected) { - if (typeof callback === 'function') { - setImmediate(callback, null, globalConnection) - return globalConnection - } else { - return shared.Promise.resolve(globalConnection) - } - } else if (typeof callback === 'function') { - onGlobalConnect.push(callback) - return globalConnection - } else { - return new shared.Promise((resolve, reject) => { - onGlobalConnect.push((err, pool) => { - if (err) { - reject(err) - } else { - resolve(pool) - } - }) - }) - } + return globalConnection.connect(callback) } /**