Do not return broken or closed clients to the pool

.gitignore

@@ -7,3 +7,4 @@ package-lock.json
 *.swp
 dist
 .DS_Store
+.vscode/

.travis.yml

@@ -10,7 +10,7 @@ env:
 node_js:
   - lts/dubnium
   - lts/erbium
-  - 13
+  - 13.6
 
 addons:
   postgresql: "10"

packages/pg-pool/index.js

@@ -25,6 +25,10 @@ class PendingItem {
   }
 }
 
+function throwOnDoubleRelease () {
+  throw new Error('Release called on client which has already been released to the pool.')
+}
+
 function promisify (Promise, callback) {
   if (callback) {
     return { callback: callback, result: undefined }
@@ -60,6 +64,18 @@ class Pool extends EventEmitter {
   constructor (options, Client) {
     super()
     this.options = Object.assign({}, options)
+
+    if (options != null && 'password' in options) {
+      // "hiding" the password so it doesn't show up in stack traces
+      // or if the client is console.logged
+      Object.defineProperty(this.options, 'password', {
+        configurable: true,
+        enumerable: false,
+        writable: true,
+        value: options.password
+      })
+    }
+
     this.options.max = this.options.max || this.options.poolSize || 10
     this.log = this.options.log || function () { }
     this.Client = this.options.Client || Client || require('pg').Client
@@ -244,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
@@ -280,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

packages/pg-pool/package.json

@@ -34,6 +34,6 @@
     "pg-cursor": "^1.3.0"
   },
   "peerDependencies": {
-    "pg": ">5.0"
+    "pg": ">=8.0"
   }
 }

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()
+  })
+})

packages/pg/Makefile

@@ -62,6 +62,4 @@ test-pool:
 
 lint:
 	@echo "***Starting lint***"
-	node -e "process.exit(Number(process.versions.node.split('.')[0]) < 8 ? 0 : 1)" \
-	  && echo "***Skipping lint (node version too old)***" \
-	  || node_modules/.bin/eslint lib
+	node_modules/.bin/eslint lib

packages/pg/lib/client.js

@@ -30,7 +30,16 @@ var Client = function (config) {
   this.database = this.connectionParameters.database
   this.port = this.connectionParameters.port
   this.host = this.connectionParameters.host
-  this.password = this.connectionParameters.password
+
+  // "hiding" the password so it doesn't show up in stack traces
+  // or if the client is console.logged
+  Object.defineProperty(this, 'password', {
+    configurable: true,
+    enumerable: false,
+    writable: true,
+    value: this.connectionParameters.password
+  })
+
   this.replication = this.connectionParameters.replication
 
   var c = config || {}

packages/pg/lib/connection-parameters.js

@@ -52,9 +52,23 @@ var ConnectionParameters = function (config) {
 
   this.user = val('user', config)
   this.database = val('database', config)
+
+  if (this.database === undefined) {
+    this.database = this.user
+  }
+
   this.port = parseInt(val('port', config), 10)
   this.host = val('host', config)
-  this.password = val('password', config)
+
+  // "hiding" the password so it doesn't show up in stack traces
+  // or if the client is console.logged
+  Object.defineProperty(this, 'password', {
+    configurable: true,
+    enumerable: false,
+    writable: true,
+    value: val('password', config)
+  })
+
   this.binary = val('binary', config)
   this.ssl = typeof config.ssl === 'undefined' ? useSsl() : config.ssl
   this.client_encoding = val('client_encoding', config)

packages/pg/lib/defaults.js

@@ -15,7 +15,7 @@ module.exports = {
   user: process.platform === 'win32' ? process.env.USERNAME : process.env.USER,
 
   // name of database to connect
-  database: process.platform === 'win32' ? process.env.USERNAME : process.env.USER,
+  database: undefined,
 
   // database user's password
   password: null,

packages/pg/lib/index.js

@@ -7,25 +7,17 @@
  * README.md file in the root directory of this source tree.
  */
 
-var util = require('util')
 var Client = require('./client')
 var defaults = require('./defaults')
 var Connection = require('./connection')
 var Pool = require('pg-pool')
-const checkConstructor = require('./compat/check-constructor')
 
 const poolFactory = (Client) => {
-  var BoundPool = function (options) {
-    // eslint-disable-next-line no-eval
-    checkConstructor('pg.Pool', 'PG-POOL-NEW', () => eval('new.target'))
-
-    var config = Object.assign({ Client: Client }, options)
-    return new Pool(config)
+  return class BoundPool extends Pool {
+    constructor (options) {
+      super(options, Client)
+    }
   }
-
-  util.inherits(BoundPool, Pool)
-
-  return BoundPool
 }
 
 var PG = function (clientConstructor) {
@@ -44,20 +36,28 @@ if (typeof process.env.NODE_PG_FORCE_NATIVE !== 'undefined') {
   module.exports = new PG(Client)
 
   // lazy require native module...the native module may not have installed
-  module.exports.__defineGetter__('native', function () {
-    delete module.exports.native
-    var native = null
-    try {
-      native = new PG(require('./native'))
-    } catch (err) {
-      if (err.code !== 'MODULE_NOT_FOUND') {
-        throw err
+  Object.defineProperty(module.exports, 'native', {
+    configurable: true,
+    enumerable: false,
+    get() {
+      var native = null
+      try {
+        native = new PG(require('./native'))
+      } catch (err) {
+        if (err.code !== 'MODULE_NOT_FOUND') {
+          throw err
+        }
+        /* eslint-disable no-console */
+        console.error(err.message)
+        /* eslint-enable no-console */
       }
-      /* eslint-disable no-console */
-      console.error(err.message)
-      /* eslint-enable no-console */
+
+      // overwrite module.exports.native so that getter is never called again
+      Object.defineProperty(module.exports, 'native', {
+        value: native
+      })
+
+      return native
     }
-    module.exports.native = native
-    return native
   })
 }

packages/pg/lib/native/client.js

@@ -43,7 +43,15 @@ var Client = module.exports = function (config) {
   // for the time being. TODO: deprecate all this jazz
   var cp = this.connectionParameters = new ConnectionParameters(config)
   this.user = cp.user
-  this.password = cp.password
+
+  // "hiding" the password so it doesn't show up in stack traces
+  // or if the client is console.logged
+  const hiddenPassword = cp.password
+  Object.defineProperty(this, 'password', {
+    enumerable: false,
+    writable: true,
+    value: hiddenPassword
+  })
   this.database = cp.database
   this.host = cp.host
   this.port = cp.port

packages/pg/package.json

@@ -51,6 +51,6 @@
   ],
   "license": "MIT",
   "engines": {
-    "node": ">= 4.5.0"
+    "node": ">= 8.0.0"
   }
 }

packages/pg/test/integration/client/configuration-tests.js

@@ -14,7 +14,7 @@ for (var key in process.env) {
 suite.test('default values are used in new clients', function () {
   assert.same(pg.defaults, {
     user: process.env.USER,
-    database: process.env.USER,
+    database: undefined,
     password: null,
     port: 5432,
     rows: 0,
@@ -54,6 +54,28 @@ suite.test('modified values are passed to created clients', function () {
   })
 })
 
+suite.test('database defaults to user when user is non-default', () => {
+  {
+    pg.defaults.database = undefined
+
+    const client = new Client({
+      user: 'foo',
+    })
+
+    assert.strictEqual(client.database, 'foo')
+  }
+
+  {
+    pg.defaults.database = 'bar'
+
+    const client = new Client({
+      user: 'foo',
+    })
+
+    assert.strictEqual(client.database, 'bar')
+  }
+})
+
 suite.test('cleanup', () => {
   // restore process.env
   for (var key in realEnv) {

packages/pg/test/integration/gh-issues/1542-tests.js

@@ -0,0 +1,25 @@
+
+"use strict"
+const helper = require('./../test-helper')
+const assert = require('assert')
+
+const suite = new helper.Suite()
+
+suite.testAsync('BoundPool can be subclassed', async () => {
+  const Pool = helper.pg.Pool;
+  class SubPool extends Pool {
+
+  }
+  const subPool = new SubPool()
+  const client = await subPool.connect()
+  client.release()
+  await subPool.end()
+  assert(subPool instanceof helper.pg.Pool)
+})
+
+suite.test('calling pg.Pool without new throws', () => {
+  const Pool = helper.pg.Pool;
+  assert.throws(() => {
+    const pool = Pool()
+  })
+})

packages/pg/test/integration/gh-issues/1992-tests.js

@@ -0,0 +1,11 @@
+
+"use strict"
+const helper = require('./../test-helper')
+const assert = require('assert')
+
+const suite = new helper.Suite()
+
+suite.test('Native should not be enumerable', () => {
+  const keys = Object.keys(helper.pg)
+  assert.strictEqual(keys.indexOf('native'), -1)
+})

packages/pg/test/integration/gh-issues/2064-tests.js

@@ -0,0 +1,32 @@
+
+"use strict"
+const helper = require('./../test-helper')
+const assert = require('assert')
+const util = require('util')
+
+const suite = new helper.Suite()
+
+const password = 'FAIL THIS TEST'
+
+suite.test('Password should not exist in toString() output', () => {
+  const pool = new helper.pg.Pool({ password })
+  const client = new helper.pg.Client({ password })
+  assert(pool.toString().indexOf(password) === -1);
+  assert(client.toString().indexOf(password) === -1);
+})
+
+suite.test('Password should not exist in util.inspect output', () => {
+  const pool = new helper.pg.Pool({ password })
+  const client = new helper.pg.Client({ password })
+  const depth = 20;
+  assert(util.inspect(pool, { depth }).indexOf(password) === -1);
+  assert(util.inspect(client, { depth }).indexOf(password) === -1);
+})
+
+suite.test('Password should not exist in json.stringfy output', () => {
+  const pool = new helper.pg.Pool({ password })
+  const client = new helper.pg.Client({ password })
+  const depth = 20;
+  assert(JSON.stringify(pool).indexOf(password) === -1);
+  assert(JSON.stringify(client).indexOf(password) === -1);
+})