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.

Author: charmander

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,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))
 }

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 () {