From fd802a385c25ad588350845485ab13e40ccf9752 Mon Sep 17 00:00:00 2001 From: Charmander <~@charmander.me> Date: Sun, 4 Dec 2016 15:20:24 -0800 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20create=20promises=20when=20call?= =?UTF-8?q?backs=20are=20provided=20(#31)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert "When connection fail, emit the error. (#28)" 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. * Don’t create promises when callbacks are provided 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. * Use Node-0.10-compatible `process.nextTick` --- index.js | 54 ++++++++++++++++++++++++++++++++------------------ test/events.js | 26 ++---------------------- test/index.js | 26 ++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 43 deletions(-) diff --git a/index.js b/index.js index f62f667d..d3551868 100644 --- a/index.js +++ b/index.js @@ -22,6 +22,32 @@ 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(function () { + cb(null, value) + }) + } + + function rejected (error) { + process.nextTick(function () { + 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 @@ -43,7 +69,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) @@ -53,15 +78,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') @@ -80,9 +107,9 @@ Pool.prototype.connect = function (cb) { if (cb) { cb(null, client, client.release) + } else { + resolve(client) } - - return resolve(client) }.bind(this)) }.bind(this)) } @@ -95,20 +122,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)) @@ -116,15 +137,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/events.js b/test/events.js index 5045cc2c..bcb523f0 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')) }) diff --git a/test/index.js b/test/index.js index d7752fce..5f6d0ad5 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 () {