From c868226a36b0ef7af46704896fe0692f671b7861 Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Thu, 27 Oct 2016 21:36:13 -0700 Subject: [PATCH 1/3] Revert "When connection fail, emit the error. (#28)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 6a7edabc22e36db7386c97ee93f08f957364f37d. The callback passed to `Pool.prototype.connect` should be responsible for handling connection errors. The `error` event is documented to be: > Emitted whenever an idle client in the pool encounters an error. This isn’t the case of an idle client in the pool; it never makes it into the pool. It also breaks tests on pg’s master because of nonspecific dependencies. --- index.js | 1 - test/events.js | 26 ++------------------------ 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/index.js b/index.js index f62f667..10d8dd0 100644 --- a/index.js +++ b/index.js @@ -43,7 +43,6 @@ Pool.prototype._create = function (cb) { if (err) { this.log('client connection error:', err) cb(err) - this.emit('error', err) } else { this.log('client connected') this.emit('connect', client) diff --git a/test/events.js b/test/events.js index 5045cc2..bcb523f 100644 --- a/test/events.js +++ b/test/events.js @@ -22,7 +22,7 @@ describe('events', function () { }) }) - it('emits "error" with a failed connection', function (done) { + it('emits "connect" only with a successful connection', function (done) { var pool = new Pool({ // This client will always fail to connect Client: mockClient({ @@ -34,29 +34,7 @@ describe('events', function () { pool.on('connect', function () { throw new Error('should never get here') }) - pool.on('error', function (err) { - if (err) done() - else done(new Error('expected failure')) - }) - pool.connect() - }) - - it('callback err with a failed connection', function (done) { - var pool = new Pool({ - // This client will always fail to connect - Client: mockClient({ - connect: function (cb) { - process.nextTick(function () { cb(new Error('bad news')) }) - } - }) - }) - pool.on('connect', function () { - throw new Error('should never get here') - }) - pool.on('error', function (err) { - if (!err) done(new Error('expected failure')) - }) - pool.connect(function (err) { + pool._create(function (err) { if (err) done() else done(new Error('expected failure')) }) From 0a838ff0821603276e9bd015d5dc084da04b0d1e Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Thu, 27 Oct 2016 22:11:19 -0700 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20create=20promises=20when=20?= =?UTF-8?q?callbacks=20are=20provided?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s incorrect to do so. One consequence is that a rejected promise will be unhandled, which is currently annoying, but also dangerous in the future: > DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. The way callbacks are used currently also causes #24 (hiding of errors thrown synchronously from the callback). One fix for that would be to call them asynchronously from inside the `new Promise()` executor: process.nextTick(cb, error); I don’t think it’s worth implementing, though, since it would still be backwards-incompatible – just less obvious about it. Also fixes a bug where the `Pool.prototype.connect` callback would be called twice if there was an error. --- index.js | 49 +++++++++++++++++++++++++++++++------------------ test/index.js | 26 ++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 18 deletions(-) diff --git a/index.js b/index.js index 10d8dd0..b883074 100644 --- a/index.js +++ b/index.js @@ -22,6 +22,28 @@ var Pool = module.exports = function (options, Client) { util.inherits(Pool, EventEmitter) +Pool.prototype._promise = function (cb, executor) { + if (!cb) { + return new this.Promise(executor) + } + + function resolved (value) { + process.nextTick(cb, null, value) + } + + function rejected (error) { + process.nextTick(cb, error) + } + + executor(resolved, rejected) +} + +Pool.prototype._promiseNoCallback = function (callback, executor) { + return callback + ? executor() + : new this.Promise(executor) +} + Pool.prototype._destroy = function (client) { if (client._destroying) return client._destroying = true @@ -52,15 +74,17 @@ Pool.prototype._create = function (cb) { } Pool.prototype.connect = function (cb) { - return new this.Promise(function (resolve, reject) { + return this._promiseNoCallback(cb, function (resolve, reject) { this.log('acquire client begin') this.pool.acquire(function (err, client) { if (err) { this.log('acquire client. error:', err) if (cb) { cb(err, null, function () {}) + } else { + reject(err) } - return reject(err) + return } this.log('acquire client') @@ -79,9 +103,9 @@ Pool.prototype.connect = function (cb) { if (cb) { cb(null, client, client.release) + } else { + resolve(client) } - - return resolve(client) }.bind(this)) }.bind(this)) } @@ -94,20 +118,14 @@ Pool.prototype.query = function (text, values, cb) { values = undefined } - return new this.Promise(function (resolve, reject) { + return this._promise(cb, function (resolve, reject) { this.connect(function (err, client, done) { if (err) { - if (cb) { - cb(err) - } return reject(err) } client.query(text, values, function (err, res) { done(err) err ? reject(err) : resolve(res) - if (cb) { - cb(err, res) - } }) }) }.bind(this)) @@ -115,15 +133,10 @@ Pool.prototype.query = function (text, values, cb) { Pool.prototype.end = function (cb) { this.log('draining pool') - return new this.Promise(function (resolve, reject) { + return this._promise(cb, function (resolve, reject) { this.pool.drain(function () { this.log('pool drained, calling destroy all now') - this.pool.destroyAllNow(function () { - if (cb) { - cb() - } - resolve() - }) + this.pool.destroyAllNow(resolve) }.bind(this)) }.bind(this)) } diff --git a/test/index.js b/test/index.js index d7752fc..5f6d0ad 100644 --- a/test/index.js +++ b/test/index.js @@ -103,6 +103,32 @@ describe('pool', function () { pool.end(done) }) }) + + it('does not create promises when connecting', function (done) { + var pool = new Pool() + var returnValue = pool.connect(function (err, client, release) { + release() + if (err) return done(err) + pool.end(done) + }) + expect(returnValue).to.be(undefined) + }) + + it('does not create promises when querying', function (done) { + var pool = new Pool() + var returnValue = pool.query('SELECT 1 as num', function (err) { + pool.end(function () { + done(err) + }) + }) + expect(returnValue).to.be(undefined) + }) + + it('does not create promises when ending', function (done) { + var pool = new Pool() + var returnValue = pool.end(done) + expect(returnValue).to.be(undefined) + }) }) describe('with promises', function () { From 1be18029d57139d35311668e71a208805e6e782a Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Thu, 27 Oct 2016 23:06:16 -0700 Subject: [PATCH 3/3] Use Node-0.10-compatible `process.nextTick` --- index.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index b883074..d355186 100644 --- a/index.js +++ b/index.js @@ -28,11 +28,15 @@ Pool.prototype._promise = function (cb, executor) { } function resolved (value) { - process.nextTick(cb, null, value) + process.nextTick(function () { + cb(null, value) + }) } function rejected (error) { - process.nextTick(cb, error) + process.nextTick(function () { + cb(error) + }) } executor(resolved, rejected)