Node-postgres: Proposal: `pool.connect` should return proxy instances

[mike-marcacci]

I'd like to propose a change to the pool package, which I'll go ahead and implement if I get the thumbs-up here.

Instead of returning "real" connection objects, it's my opinion that the pool should return a "proxy" to the connection object. That is, not necessarily an actual JS Proxy object, but an object with the same API as a connection, but unique to the particular acquisition.

There appear to already be protections against "double release" of a connection back to the pool, but no protections against _use after release_. This would solve that quite elegantly.

Let's take the following bad (but surprisingly common) code:

async function loadThings(ids) {
  const connection = await pool.connect();
  try {
    await connection.query('BEGIN');

    await Promise.all(ids.map(async (id) => {
      // First performs some fallible async operation
      const thing = await fetchThingById(id);

      // Performs a write operation afterwords.
      connection.query('INSERT INTO things (id, content) VALUES ($, $)', [id, thing]);
    }))
  } catch (error) {
    await connection.query('ROLLBACK');
    throw error;
  } finally {
    connection.release();
  }
}

The problem here is that a call to fetchThingById can fail, causing the the transaction to get rolled back and the connection to get released. However, the acquired connection is still referenced by the other pending operations. This means that an INSERT operation is likely to happen _outside_ the transaction (or within a _different_ transaction if the connection is reacquired).

Of course, this is misuse by the user of this library and is clearly outside its intended behavior. However, it's incredibly easy to write code with this kind of problem, and I think the pool is the best place to fix this.

If pool.connect was to instead return an object that _wraps_ the "real" connection object but shares its API, referencing the real connection internally and removing that reference on release, subsequent calls to its methods would fail instead of potentially causing unexpected and "partial" writes.

An error like "Unable to use a connection after it has been released" in such a case would be a solid alternative to undefined behavior.

[charmander]

I'd like to propose a change to the pool package, which I'll go ahead and implement if I get the thumbs-up here.

Please do! IIRC everyone’s for it.