Don’t create promises when callbacks are provided (#31)

* Revert "When connection fail, emit the error. (#28)"

This reverts commit 6a7edab.

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`

Author: charmander

Diff for: 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,36 +122,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))
 }

Diff for: 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 for: 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 () {