connection error prevents pool from ending connection

[afanasy]

This doesn't work as expected (pool doesn't end, script doesn't exit)

pool.getConnection(function (err, conn) {
  conn.query('').
    on('end', function () {
      conn.release()
      pool.end()
    })
})

however, this works (pool closes connections and script exits)

pool.getConnection(function (err, conn) {
  conn.query('').
    on('error', function () {}).
    on('end', function () {
      conn.release()
      pool.end()
    })
})

Thanks for the great lib!

[afanasy]

conn.query('') = any invalid query

[dougwilson]

Ah ha, I see now. So what is happening is is a sequence, like query, doesn't have an error handler (either an error event listener or a callback function), then the error is not handed to the sequence, but as a consequence, the end event for the sequence will never fire. Seems like a bug to me, since we're not being consistent on when the end event is emitted. Since it currently emits on an error, it should still emit in this case as well :)

[afanasy]

@dougwilson thanks! yes, that would be more expected behaviour.

[manuguerra]

Actually the end event is being fired; the problem is that, since there are no listeners to the error event, it bubbles up to PoolConnection, which, in turn, will call _removeFromPool. This will remove any references to the connection, and when we try to end the pool, the connection won't be ended/destroyed, causing a stall.
Do we really need to call _removeFromPool on error, since it will eventually be called on end events?

[manuguerra]

In my previous pool request, I assumed that all errors would trigger end events, but that may not always be the case. All fatal errors trigger end events, but it's not necessarily true for other types of errors.
So I thought about making a slight modification of the proposed fix. In PoolConnection, we would have:

this.on('error', function() {
  self._removeFromPool();
  self.destroy();
});

This also fixes the dangling connection issue and passes the tests.
What do you think, @dougwilson? Is it ok to always destroy the connection here or only if it is in a specific state, like protocol_error (which is the case for an invalid query)?

[dougwilson]

I believe this issue got fixed at some point, based on me not being able to reproduce anymore. If someone can reproduce with version 2.6.1+, please open a new issue/PR and let me know!