Skip to content

Commit

Permalink
Don’t create promises when callbacks are provided
Browse files Browse the repository at this point in the history
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 brianc#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.
  • Loading branch information
charmander committed Oct 28, 2016
1 parent c868226 commit 0a838ff
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 18 deletions.
49 changes: 31 additions & 18 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
Expand All @@ -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))
}
Expand All @@ -94,36 +118,25 @@ 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))
}

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))
}
26 changes: 26 additions & 0 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down

0 comments on commit 0a838ff

Please sign in to comment.