Prevent reuse of released client instances

[kjdelisle]

I noticed that if I continue making use of a client reference after calling done(), like so:

pool.connect(function (err, client, done) {
  if (err) {
    return console.error('error fetching client from pool', err);
  }
  function execute(input, cb) {
    client.query('SELECT $1::int AS number', [input], function (err, result) {
      console.log('Current number of connections in use: %s', pool.pool._inUseObjects.length);
      console.log('Current number of available connections: %s', pool.pool._availableObjects.length);
      process.nextTick(function() {
        return cb(err, result);
      });
    });
  }

  async.each([1, 2, 3], function (i, cb) {
    execute(i, function(err) {
      if (err) console.error('Oh noes: %s', err);
      done();
    });
  });
});

...that it would release the items to the pool, but still make use of them:

Current number of connections in use: 1
Current number of available connections: 0
Current number of connections in use: 0
Current number of available connections: 1
Current number of connections in use: 0
Current number of available connections: 1

I would still expect a user of this library to be careful about reusing connection instances that they've released to the pool, but if you were to use an intermediate object reference that prevents direct access to those connection instances, you could invalidate those references by setting a flag on client.release() that prevents people from calling things like client.query(...).

[charmander]

Use pool.query and Bluebird resource management.

[kjdelisle]

@charmander Isn't the whole idea of a connection pool that it limits the ability of calling code to arbitrarily create and utilize connections to a database? As I mentioned initially, you can follow best practices to avoid the problem, but wouldn't it make a lot of sense to add something that throws errors when you attempt to use a resource you've given back to the pool?

Additionally, if I want to perform a series of distinct queries/transactions against a database, using pool.query would add the overhead of releasing connections back to the pool and asking for new ones, which could be especially cumbersome if the pool is currently exhausted and that particular set of actions must now queue up to receive a resource it could've hung onto.

[charmander]

Sorry, to be specific: they’re for the two different cases.

pg.Pool.prototype.connectResource = function () {
  return this.connect().disposer(function (client) {
    client.release();
  });
};

function singleQuery(db, input) {
  return db.query('SELECT $1::int AS number', [input]);
}

function multipleQueries(db) {
  return Promise.using(db.connectResource(), function (client) {
    return Promise.map([1, 2, 3], function (input) {
      return singleQuery(client, input);
    });
  });
}

[kjdelisle]

Your example is a good pattern, but again, it's not about the idea that you can just "code better" to avoid the problem. It's that if you're interested in holding a connection from the pool for the right reasons, and you were to screw up somewhere down a long and potentially convoluted chain of calls that it won't do what a connection pool is meant to do and stop you.

Your example would limit the value of the connection pool to its queuing functionality when the pool is exhausted.

I think I'm going to open a PR for this soon.

[charmander]

It's that if you're interested in holding a connection from the pool for the right reasons

Which reasons?

[kjdelisle]

Maybe I want to run 500 separate transactions in series and don't want to incur the overhead of constantly releasing/acquiring a connection from the pool? It might also be that those transactions aren't just 500 of the same function, running over and over (maybe they're pulled from a unit-of-work queue or something to that effect).

There's any number of potential use-cases for not immediately surrendering your hold on the connection instance after a single query.

[charmander]

Maybe I want to run 500 separate transactions in series and don't want to incur the overhead of constantly releasing/acquiring a connection from the pool?

You might be overestimating the overhead:

'use strict';

var pg = require('./');

var pool = new pg.Pool();

var COUNT = 10000;

function release(client) {
  client.release();
}

pool.connect().then(function (client) {
  client.release();

  var start = process.hrtime();
  var i = 0;

  (function acquire() {
    if (i++ === COUNT) {
      var time = process.hrtime(start);
      console.log((time[0] + time[1] * 1e-9).toFixed(3));
      pool.end();
      return;
    }

    pool.connect()
      .then(release)
      .then(acquire);
  })();
});

10,000 release/acquires, 42 ms. That’s the point of a pool.

[charmander]

(To be clear, I’m not against this being implemented; extra safety is always nice. I do think any instances of this problem are indicative of questionable design on the user’s part, though.)

[kjdelisle]

Actually, I just discovered something interesting while working on my branch...
Subsequent calls to the same client object will fail when using Promises instead of callbacks with TypeError: client.release is not a function

It makes sense because the Promise version is handing back the reference to the object itself, while the callback version is receiving its own distinct reference to the function:
https://github.com/brianc/node-pg-pool/blob/master/index.js#L139

So even though there's code in there to delete the client.release reference, it's only going to work for the Promise version since any calling code will still retain its reference (in the form of the done parameter).

So, technically, if you're using Promises, you're already protected from using released connection objects (just not in a way that's crystal clear).

Sorry, wasn't thinking about it enough. This still lets you run one more query against the now-released resource. It's only on the release the 2nd time around that you run aground.

[vitaly-t]

It makes sense because the Promise version is handing back the reference to the object itself, while the callback version is receiving its own distinct reference to the function:

I believe it makes sense because promises are based on the use of process ticks, i.e. delays, and once a delay is created, the pool gets released, but not immediately.

I fully support @kjdelisle, this library should support simple diagnostics when someone tries to use a released pool connection. This would let you easily spot a fault in your code 😉