Write tests for change

brianc · brianc · commit 48ed1902987e · 2020-01-23T11:31:15.000-06:00
diff --git 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
@@ -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
@@ -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()
+  })
+})
