From 589a8339ce088b16777e9f5f635d2f551b8a5bbd Mon Sep 17 00:00:00 2001 From: Sean Garner Date: Tue, 12 May 2015 07:57:28 +0100 Subject: [PATCH 1/4] switch user back to pool default on connection release --- Changes.md | 7 +++++- Readme.md | 12 +++++++--- lib/Pool.js | 30 ++++++++++++++++++++++-- lib/PoolConnection.js | 3 ++- test/unit/pool/test-change-user-eject.js | 15 +++++++++--- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/Changes.md b/Changes.md index c6be4e517..aa3f17cd1 100644 --- a/Changes.md +++ b/Changes.md @@ -4,7 +4,12 @@ This file is a manually maintained list of changes for each release. Feel free to add your changes here when sending pull requests. Also send corrections if you spot any mistakes. -## HEAD +## NEXT_MINOR + +* Pool resets credentials to pool configuration on returned connection + instead of destroying after changeUser is called #837 + +## v2.7.0 (2015-05-05) * Provide static require analysis diff --git a/Readme.md b/Readme.md index 29caee8e8..02ed5693f 100644 --- a/Readme.md +++ b/Readme.md @@ -390,7 +390,7 @@ addition to those options pools accept a few extras: ### connection -The pool will emit a `connection` event when a new connection is made within the pool. +The pool will emit a `connection` event when a new connection is made within the pool. If you need to set session variables on the connection before it gets used, you can listen to the `connection` event. @@ -459,7 +459,7 @@ poolCluster.getConnection('MASTER', function (err, connection) {}); // Target Group : SLAVE1-2, Selector : order // If can't connect to SLAVE1, return SLAVE2. (remove SLAVE1 in the cluster) poolCluster.on('remove', function (nodeId) { - console.log('REMOVED NODE : ' + nodeId); // nodeId = SLAVE1 + console.log('REMOVED NODE : ' + nodeId); // nodeId = SLAVE1 }); poolCluster.getConnection('SLAVE*', 'ORDER', function (err, connection) {}); @@ -479,7 +479,7 @@ poolCluster.end(function (err) { ## PoolCluster Option * `canRetry`: If `true`, `PoolCluster` will attempt to reconnect when connection fails. (Default: `true`) -* `removeNodeErrorCount`: If connection fails, node's `errorCount` increases. +* `removeNodeErrorCount`: If connection fails, node's `errorCount` increases. When `errorCount` is greater than `removeNodeErrorCount`, remove a node in the `PoolCluster`. (Default: `5`) * `restoreNodeTimeout`: If connection fails, specifies the number of milliseconds before another connection attempt will be made. If set to `0`, then node will be @@ -519,6 +519,12 @@ The available options for this feature are: A sometimes useful side effect of this functionality is that this function also resets any connection state (variables, transactions, etc.). +In the context of connection being returned to a Pool the connection will be +reset back to the defaults for the pool before the connection is released to +another `getConnection()` call. When this happens the pool will emit +`connection` to give the listener a chance to setup state on the connection +again since it will have been reset. + Errors encountered during this operation are treated as fatal connection errors by this module. diff --git a/lib/Pool.js b/lib/Pool.js index 5c589eb00..da464818c 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -109,8 +109,11 @@ Pool.prototype.releaseConnection = function releaseConnection(connection) { } if (connection._purge) { - // purge connection from pool - this._purgeConnection(connection); + // restore the switched user + this.restoreUser(connection, function (err) { + if (err) return connection.destroy(); + connection._pool.releaseConnection(connection); + }); return; } else if (this._freeConnections.indexOf(connection) !== -1) { // connection already in free connection pool @@ -247,6 +250,29 @@ Pool.prototype._removeConnection = function(connection) { this.releaseConnection(connection); }; +Pool.prototype.restoreUser = function restoreUser(connection, callback) { + var poolConfig = this.config.connectionConfig; + var pool = this; + + var config = connection.config; + var user = poolConfig.user; + var database = poolConfig.database; + var password = poolConfig.password; + + // only when credentials have been changed from the pool defaults + if (config.user !== user || config.database !== database || config.password !== password) { + connection.changeUser({ user: user, database: database, password: password }, function (err) { + if (err) return callback(err); + // emit as if it were a new connection so consumers can setup session state again + pool.emit('connection', connection); + callback(null); + }); + connection._purge = false; + } else { + process.nextTick(callback); + } +} + Pool.prototype.escape = function(value) { return mysql.escape(value, this.config.connectionConfig.stringifyObjects, this.config.connectionConfig.timezone); }; diff --git a/lib/PoolConnection.js b/lib/PoolConnection.js index 4189053c1..eacc73b86 100644 --- a/lib/PoolConnection.js +++ b/lib/PoolConnection.js @@ -8,7 +8,7 @@ inherits(PoolConnection, Connection); function PoolConnection(pool, options) { Connection.call(this, options); this._pool = pool; - this._purge = false + this._purge = false; // When a fatal error occurs the connection's protocol ends, which will cause // the connection to end as well, thus we only need to watch for the end event @@ -29,6 +29,7 @@ PoolConnection.prototype.changeUser = function changeUser(options, callback) { PoolConnection.prototype.release = function release() { var pool = this._pool; + var connection = this; if (!pool || pool._closed) { return; diff --git a/test/unit/pool/test-change-user-eject.js b/test/unit/pool/test-change-user-eject.js index 325b7d1de..9c3ef932e 100644 --- a/test/unit/pool/test-change-user-eject.js +++ b/test/unit/pool/test-change-user-eject.js @@ -9,10 +9,15 @@ var pool = common.createPool({ var closed = 0; var server = common.createFakeServer(); var thread = 0; +var connections = 0; server.listen(common.fakeServerPort, function(err) { assert.ifError(err); + pool.on('connection', function(conn) { + connections++; + }); + var conn0; var threadId; pool.getConnection(function(err, conn) { @@ -22,6 +27,7 @@ server.listen(common.fakeServerPort, function(err) { threadId = conn.threadId; }); + pool.getConnection(function(err, conn) { assert.ifError(err); assert.ok(conn.threadId === 1 || conn.threadId === 2); @@ -38,17 +44,20 @@ server.listen(common.fakeServerPort, function(err) { pool.getConnection(function(err, conn1) { assert.ifError(err); - assert.strictEqual(conn1.threadId, 3); + assert.ok(conn1.threadId === 1 || conn1.threadId === 2); + assert.strictEqual(conn1.config.user, 'user_1'); pool.getConnection(function(err, conn2) { assert.ifError(err); - assert.strictEqual(conn2.threadId, threadId); + assert.ok(conn2.threadId === 1 || conn2.threadId === 2); + assert.strictEqual(conn1.config.user, 'user_1'); conn1.release(); conn2.release(); pool.end(function(err) { assert.ifError(err); - assert.strictEqual(closed, 3); + assert.strictEqual(connections, 3); + assert.strictEqual(closed, 2); server.destroy(); }); }); From 1fcba727c26673bda67efb18e345e0a09cbd523e Mon Sep 17 00:00:00 2001 From: Sean Garner Date: Thu, 2 Jul 2015 19:01:13 +0100 Subject: [PATCH 2/4] use purgeConnection instead of destroy on restoreUser error --- lib/Pool.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Pool.js b/lib/Pool.js index da464818c..0d7949e60 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -111,7 +111,7 @@ Pool.prototype.releaseConnection = function releaseConnection(connection) { if (connection._purge) { // restore the switched user this.restoreUser(connection, function (err) { - if (err) return connection.destroy(); + if (err) return this._purgeConnection(connection); connection._pool.releaseConnection(connection); }); return; From 8989512d6ccb84e72f8c42590eca258dfd9cadc4 Mon Sep 17 00:00:00 2001 From: Sean Garner Date: Sat, 4 Jul 2015 08:27:59 +0100 Subject: [PATCH 3/4] test connection is running user it says it is --- test/unit/pool/test-change-user-eject.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/test/unit/pool/test-change-user-eject.js b/test/unit/pool/test-change-user-eject.js index 9c3ef932e..ace718380 100644 --- a/test/unit/pool/test-change-user-eject.js +++ b/test/unit/pool/test-change-user-eject.js @@ -23,6 +23,10 @@ server.listen(common.fakeServerPort, function(err) { pool.getConnection(function(err, conn) { assert.ifError(err); assert.ok(conn.threadId === 1 || conn.threadId === 2); + conn.query('SELECT CURRENT_USER()', function (err, rows) { + assert.ifError(err); + assert.strictEqual(rows[0]['CURRENT_USER()'].indexOf('user_1'), 0); + }); conn0 = conn; threadId = conn.threadId; }); @@ -37,6 +41,10 @@ server.listen(common.fakeServerPort, function(err) { conn.changeUser({user: 'user_2'}, function(err) { assert.ifError(err); assert.strictEqual(conn.threadId, threadId); + conn.query('SELECT CURRENT_USER()', function (err, rows) { + assert.ifError(err); + assert.strictEqual(rows[0]['CURRENT_USER()'].indexOf('user_2'), 0); + }); conn.release(); conn0.release(); }); @@ -46,13 +54,21 @@ server.listen(common.fakeServerPort, function(err) { assert.ifError(err); assert.ok(conn1.threadId === 1 || conn1.threadId === 2); assert.strictEqual(conn1.config.user, 'user_1'); + conn1.query('SELECT CURRENT_USER()', function (err, rows) { + assert.ifError(err); + assert.strictEqual(rows[0]['CURRENT_USER()'].indexOf('user_1'), 0); + }); pool.getConnection(function(err, conn2) { assert.ifError(err); assert.ok(conn2.threadId === 1 || conn2.threadId === 2); assert.strictEqual(conn1.config.user, 'user_1'); - conn1.release(); - conn2.release(); + conn2.query('SELECT CURRENT_USER()', function (err, rows) { + assert.ifError(err); + assert.strictEqual(rows[0]['CURRENT_USER()'].indexOf('user_1'), 0); + conn1.release(); + conn2.release(); + }); pool.end(function(err) { assert.ifError(err); From 8a94f9b66515b141d4f4e85e7b84c7bfe798aa9e Mon Sep 17 00:00:00 2001 From: Sean Garner Date: Thu, 9 Jul 2015 23:06:12 +0100 Subject: [PATCH 4/4] fire changeUser when returned connection has different charset than pool --- lib/Pool.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/Pool.js b/lib/Pool.js index 0d7949e60..e740c7a5e 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -251,17 +251,14 @@ Pool.prototype._removeConnection = function(connection) { }; Pool.prototype.restoreUser = function restoreUser(connection, callback) { - var poolConfig = this.config.connectionConfig; var pool = this; + var poolConfig = pool.config.connectionConfig; var config = connection.config; - var user = poolConfig.user; - var database = poolConfig.database; - var password = poolConfig.password; // only when credentials have been changed from the pool defaults - if (config.user !== user || config.database !== database || config.password !== password) { - connection.changeUser({ user: user, database: database, password: password }, function (err) { + if (config.user !== poolConfig.user || config.database !== poolConfig.database || config.password !== poolConfig.password || config.charset !== poolConfig.charset) { + connection.changeUser(poolConfig, function (err) { if (err) return callback(err); // emit as if it were a new connection so consumers can setup session state again pool.emit('connection', connection);