diff --git a/packages/pg-pool/index.js b/packages/pg-pool/index.js index aed6656f..e144bb83 100644 --- a/packages/pg-pool/index.js +++ b/packages/pg-pool/index.js @@ -25,31 +25,10 @@ class PendingItem { } } -function throwOnRelease () { +function throwOnDoubleRelease () { throw new Error('Release called on client which has already been released to the pool.') } -function release (client, err) { - client.release = throwOnRelease - if (err || this.ending || !client._queryable || client._ending) { - this._remove(client) - this._pulseQueue() - return - } - - // idle timeout - let tid - if (this.options.idleTimeoutMillis) { - tid = setTimeout(() => { - this.log('remove idle client') - this._remove(client) - }, this.options.idleTimeoutMillis) - } - - this._idle.push(new IdleItem(client, tid)) - this._pulseQueue() -} - function promisify (Promise, callback) { if (callback) { return { callback: callback, result: undefined } @@ -281,7 +260,7 @@ class Pool extends EventEmitter { client.release = (err) => { if (released) { - throw new Error('Release called on client which has already been released to the pool.') + throwOnDoubleRelease() } released = true @@ -317,7 +296,8 @@ class Pool extends EventEmitter { _release (client, idleListener, err) { client.on('error', idleListener) - if (err || this.ending) { + // TODO(bmc): expose a proper, public interface _queryable and _ending + if (err || this.ending || !client._queryable || client._ending) { this._remove(client) this._pulseQueue() return diff --git a/packages/pg-pool/test/error-handling.js b/packages/pg-pool/test/error-handling.js index 4bdb2f50..72d97ede 100644 --- a/packages/pg-pool/test/error-handling.js +++ b/packages/pg-pool/test/error-handling.js @@ -181,40 +181,6 @@ describe('pool error handling', function () { }) }) - describe('releasing a not queryable client', () => { - it('removes the client from the pool', (done) => { - const pool = new Pool({ max: 1 }) - const connectionError = new Error('connection failed') - - pool.once('error', () => { - // Ignore error on pool - }) - - pool.connect((err, client) => { - expect(err).to.be(undefined) - - client.once('error', (err) => { - expect(err).to.eql(connectionError) - - // Releasing the client should remove it from the pool, - // whether called with an error or not - client.release() - - // Verify that the pool is still usuable and new client has been - // created - pool.query('SELECT $1::text as name', ['brianc'], (err, res) => { - expect(err).to.be(undefined) - expect(res.rows).to.eql([{ name: 'brianc' }]) - - pool.end(done) - }) - }) - - client.emit('error', connectionError) - }) - }) - }) - describe('pool with lots of errors', () => { it('continues to work and provide new clients', co.wrap(function* () { const pool = new Pool({ max: 1 }) diff --git a/packages/pg-pool/test/releasing-clients.js b/packages/pg-pool/test/releasing-clients.js new file mode 100644 index 00000000..8fccde36 --- /dev/null +++ b/packages/pg-pool/test/releasing-clients.js @@ -0,0 +1,54 @@ +const Pool = require('../') + +const expect = require('expect.js') +const net = require('net') + +describe('releasing clients', () => { + it('removes a client which cannot be queried', async () => { + // make a pool w/ only 1 client + const pool = new Pool({ max: 1 }) + expect(pool.totalCount).to.eql(0) + const client = await pool.connect() + expect(pool.totalCount).to.eql(1) + expect(pool.idleCount).to.eql(0) + // reach into the client and sever its connection + client.connection.end() + + // wait for the client to error out + const err = await new Promise((resolve) => client.once('error', resolve)) + expect(err).to.be.ok() + expect(pool.totalCount).to.eql(1) + expect(pool.idleCount).to.eql(0) + + // try to return it to the pool - this removes it because its broken + client.release() + expect(pool.totalCount).to.eql(0) + expect(pool.idleCount).to.eql(0) + + // make sure pool still works + const { rows } = await pool.query('SELECT NOW()') + expect(rows).to.have.length(1) + await pool.end() + }) + + it('removes a client which is ending', async () => { + // make a pool w/ only 1 client + const pool = new Pool({ max: 1 }) + expect(pool.totalCount).to.eql(0) + const client = await pool.connect() + expect(pool.totalCount).to.eql(1) + expect(pool.idleCount).to.eql(0) + // end the client gracefully (but you shouldn't do this with pooled clients) + client.end() + + // try to return it to the bool + client.release() + expect(pool.totalCount).to.eql(0) + expect(pool.idleCount).to.eql(0) + + // make sure pool still works + const { rows } = await pool.query('SELECT NOW()') + expect(rows).to.have.length(1) + await pool.end() + }) +})