client.end() no longer releases connection back into pool in v5.2.1

[watson]

The previous latest 5.x version was 5.1.0 which used an internal pool object (here's a diff). In 5.2.1, which was released a few days ago, this was replaced with pg-pool.

Here's a test which you can run on 5.1.0 and it passes, but it fails if on 5.2.1 where it will never end because the pg.connect callback isn't called after 10 connections have been opened:

var pg = require('pg')

var conString = 'postgres://localhost/<PUT-DATABASE-NAME-HERE>'
var n = 0

setInterval(function () {
  var nn = ++n
  console.log('start', nn)

  pg.connect(conString, function (err, client, done) {
    if (err) throw err
    console.log('done', nn)
    client.end()
    if (nn > 10) {
      console.log('success!')
      process.exit()
    }
  })
}, 100)

If I instead of client.end() call done(), then it works as expected. But calling client.end() used to work before the v5.2.1 release.

[brianc]

Hmmm I see - removing that behavior was an accidental backwards compatibility break between 5.1.0 and 5.2.0 that happened a long time ago. Looking at npm info pg it looks to me like the most recent 5.x version was actually 5.2.0 which is why I applied the security patch from here to 5.2.1 and not 5.1.1. Where are you seeing that the previous latest 5.x version was 5.1.0?

---

FWIW if you want to remove a client from the pool in newer versions the recommended way to do so is by passing a truthy value to the done callback:

https://node-postgres.com/api/pool#release-err-error-

The thinking is that you shouldn't call client.end() on a client you didn't call client.connect() on yourself. I think right now if you call client.end() on a pooled client using pg-pool the pool will have no idea the client has been ended & will keep it around...which is not very user friendly. It might be worthwhile to make client.end remove clients from the pool in the future as well...I'll have to think about that API a bit more. regardless - doing nothing and keeping ended clients around in the pool is not the right behavior.

[charmander]

But calling client.end() used to work before the v5.2.1 release.

Calling client.end() is a mistake that defeats the point of a pool; backwards compatibility can’t take all API misuses into account.

If you’re unable to fix this and the potential vulnerability patched in 5.2.1 doesn’t affect you, stay on 5.1.0.

[brianc]

@charmander I agree it breaks encapsulation in a way if you call .end on a client you didn't manually create and call .connect on yourself. It's kinda like the concept of calling free on some memory you didn't alloc yourself - generally don't do that.

My main concern going forward is I think right now if you do that in the most recent version of pg the pool will have no idea a client is ended & keep it around. Maybe it'd be best to make that method throw if you call it on a pooled client, or do a noop and print a warning out to the console, or do the old behavior of removing the client from the pool. I think any of those are probably better than a silent ending and keeping the 'dead' client in the pool. What do you think?

[charmander]

@brianc If brianc/node-pg-pool#50 is implemented with some sort of PoolClient type, it would only have a release and not an end, right? If you’re okay with that approach I have a PR more or less ready.

[sehrope]

The standard way of handling this in Java/JDBC is to override the .close() method of the connection to return it to the pool. From the application's perspective it thinks it's dealing with a brand new connection and when it gets closed the pool reclaims it (rather than closing the underlying real connection). This is generally done with a proxy object that wraps all the public method of the connection.

The proxy approach also has the advantage of providing a place to catch use-after-close situations. The proxy can maintain an internal state object and if marked as "CLOSED" then any invocations of .query(...), multiple .close() calls, or really any subsequent usage could immediately throw an Error. This would cover brianc/node-pg-pool/issues/50.

The only downside is a bit more object creation (one more per checked out connection) and an extra function call for the indirection (i.e. app calls proxy, proxy calls real function on raw connection). Compared to the time it takes to execute a roundtrip a query to a DB I think it'd be insignificant.

So with this approach the "close" method would be called end() to match with the non-pool usage. To maintain backwards compatibility the release() could be an alias for it. That way none of this wouldn't be a breaking change for people who are properly using pooled connections. The only break would be for users that are using pooled connections after they invoke release() and I'd consider those beyond broken already.

[charmander]

Moving this to a new issue to keep things neat, as the original is addressed:

client.end() no longer releases connection back into pool in v5.2.1

It never did that, and it just breaks differently between 5.1.0 and 5.2.0.