emit "connect" event only on success and avoid double callback (brianc#22)

* fail: "connect" event only on success

Double callback invocation will also cause this to fail.

* avoid double callback: _create

If `client.connect` returns an error, then the callback for `Pool#_create` is only invoked once. Also the `connect` event is only emitted on a successful connection, the client is otherwise rather useless.

* legacy compat; don't use Object.assign

* legacy compat; events.EventEmitter

Author: cody-greene

index.js

@@ -40,13 +40,14 @@ Pool.prototype._create = function (cb) {
   }.bind(this))
 
   client.connect(function (err) {
-    this.log('client connected')
-    this.emit('connect', client)
     if (err) {
       this.log('client connection error:', err)
       cb(err)
+    } else {
+      this.log('client connected')
+      this.emit('connect', client)
+      cb(null, client)
     }
-    cb(err, err ? null : client)
   }.bind(this))
 }
 

test/events.js

@@ -1,8 +1,8 @@
 var expect = require('expect.js')
-
+var EventEmitter = require('events').EventEmitter
 var describe = require('mocha').describe
 var it = require('mocha').it
-
+var objectAssign = require('object-assign')
 var Pool = require('../')
 
 describe('events', function () {
@@ -22,6 +22,24 @@ describe('events', function () {
     })
   })
 
+  it('emits "connect" only with a successful 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._create(function (err) {
+      if (err) done()
+      else done(new Error('expected failure'))
+    })
+  })
+
   it('emits acquire every time a client is acquired', function (done) {
     var pool = new Pool()
     var acquireCount = 0
@@ -43,3 +61,11 @@ describe('events', function () {
     }, 40)
   })
 })
+
+function mockClient (methods) {
+  return function () {
+    var client = new EventEmitter()
+    objectAssign(client, methods)
+    return client
+  }
+}