fix(pg, pg-pool): clamp timer delays to what setTimeout supports

packages/pg-pool/index.js

@@ -3,6 +3,9 @@ const EventEmitter = require('events').EventEmitter
 
 const NOOP = function () {}
 
+// setTimeout() treats a delay above 2^31-1 ms (about 24.8 days) as 1 ms, so larger values must be clamped
+const MAX_TIMEOUT = 2147483647
+
 const removeWhere = (list, predicate) => {
   const i = list.findIndex(predicate)
 
@@ -216,13 +219,14 @@ class Pool extends EventEmitter {
       const pendingItem = new PendingItem(queueCallback)
 
       // set connection timeout on checking out an existing client
+      const timeout = Math.min(this.options.connectionTimeoutMillis, MAX_TIMEOUT)
       const tid = setTimeout(() => {
         // remove the callback from pending waiters because
         // we're going to call it with a timeout error
         removeWhere(this._pendingQueue, (i) => i.callback === queueCallback)
         pendingItem.timedOut = true
         response.callback(new Error('timeout exceeded when trying to connect'))
-      }, this.options.connectionTimeoutMillis)
+      }, timeout)
 
       if (tid.unref) {
         tid.unref()
@@ -248,6 +252,7 @@ class Pool extends EventEmitter {
     let tid
     let timeoutHit = false
     if (this.options.connectionTimeoutMillis) {
+      const timeout = Math.min(this.options.connectionTimeoutMillis, MAX_TIMEOUT)
       tid = setTimeout(() => {
         if (client.connection) {
           this.log('ending client due to timeout')
@@ -259,7 +264,7 @@ class Pool extends EventEmitter {
           // force kill the node driver, and let libpq do its teardown
           client.end()
         }
-      }, this.options.connectionTimeoutMillis)
+      }, timeout)
     }
 
     this.log('connecting new client')
@@ -310,6 +315,7 @@ class Pool extends EventEmitter {
 
   _afterConnect(client, pendingItem, idleListener) {
     if (this.options.maxLifetimeSeconds !== 0) {
+      const lifetime = Math.min(this.options.maxLifetimeSeconds * 1000, MAX_TIMEOUT)
       const maxLifetimeTimeout = setTimeout(() => {
         this.log('ending client due to expired lifetime')
         this._expired.add(client)
@@ -322,7 +328,7 @@ class Pool extends EventEmitter {
             false
           )
         }
-      }, this.options.maxLifetimeSeconds * 1000)
+      }, lifetime)
 
       maxLifetimeTimeout.unref()
       client.once('end', () => clearTimeout(maxLifetimeTimeout))
@@ -407,12 +413,13 @@ class Pool extends EventEmitter {
     // idle timeout
     let tid
     if (this.options.idleTimeoutMillis && this._isAboveMin()) {
+      const idleTimeout = Math.min(this.options.idleTimeoutMillis, MAX_TIMEOUT)
       tid = setTimeout(() => {
         if (this._isAboveMin()) {
           this.log('remove idle client')
           this._remove(client, this._pulseQueue.bind(this))
         }
-      }, this.options.idleTimeoutMillis)
+      }, idleTimeout)
 
       if (this.options.allowExitOnIdle) {
         // allow Node to exit if this is all that's left

packages/pg-pool/test/timer-overflow.js

@@ -0,0 +1,91 @@
+'use strict'
+const EventEmitter = require('events').EventEmitter
+const expect = require('expect.js')
+
+const describe = require('mocha').describe
+const it = require('mocha').it
+
+const Pool = require('../')
+
+// setTimeout() treats a delay above 2^31-1 ms as 1 ms. These tests use a mock client so they do not need a database.
+// Each one would fail within a few milliseconds if the pool passed an out-of-range delay to setTimeout.
+const wait = (time) => new Promise((resolve) => setTimeout(resolve, time))
+
+class MockClient extends EventEmitter {
+  constructor(options) {
+    super()
+    this.connectDelay = options.connectDelay || 0
+    this._queryable = true
+    this._ending = false
+  }
+
+  connect(cb) {
+    setTimeout(() => cb(null), this.connectDelay)
+  }
+
+  isConnected() {
+    return false
+  }
+
+  end(cb) {
+    this._ending = true
+    this.emit('end')
+    if (cb) process.nextTick(cb)
+  }
+
+  unref() {}
+}
+
+const newPool = (options) => new Pool({ Client: MockClient, ...options })
+
+describe('timers with values above 2^31-1 ms', () => {
+  it('maxLifetimeSeconds does not expire a client straight away', async () => {
+    // 2592000 s is 30 days, which is 2592000000 ms
+    const pool = newPool({ maxLifetimeSeconds: 2592000 })
+    const client = await pool.connect()
+    client.release()
+    await wait(50)
+    expect(pool.totalCount).to.equal(1)
+    expect(pool.expiredCount).to.equal(0)
+    await pool.end()
+  })
+
+  it('idleTimeoutMillis does not remove an idle client straight away', async () => {
+    const pool = newPool({ idleTimeoutMillis: 3000000000 })
+    const client = await pool.connect()
+    client.release()
+    await wait(50)
+    expect(pool.idleCount).to.equal(1)
+    expect(pool.totalCount).to.equal(1)
+    await pool.end()
+  })
+
+  it('idleTimeoutMillis of Infinity does not remove an idle client straight away', async () => {
+    const pool = newPool({ idleTimeoutMillis: Infinity })
+    const client = await pool.connect()
+    client.release()
+    await wait(50)
+    expect(pool.idleCount).to.equal(1)
+    expect(pool.totalCount).to.equal(1)
+    await pool.end()
+  })
+
+  it('connectionTimeoutMillis does not time out a slow connect straight away', async () => {
+    const pool = newPool({ connectionTimeoutMillis: Math.pow(2, 31), connectDelay: 50 })
+    const client = await pool.connect()
+    client.release()
+    expect(pool.totalCount).to.equal(1)
+    await pool.end()
+  })
+
+  it('connectionTimeoutMillis does not time out a queued request straight away', async () => {
+    const pool = newPool({ connectionTimeoutMillis: Math.pow(2, 31), max: 1 })
+    const first = await pool.connect()
+    const queued = pool.connect()
+    await wait(50)
+    first.release()
+    const second = await queued
+    second.release()
+    await pool.end()
+  })
+})

packages/pg/lib/client.js

@@ -10,6 +10,9 @@ const defaults = require('./defaults')
 const Connection = require('./connection')
 const crypto = require('./crypto/utils')
 
+// setTimeout() treats a delay above 2^31-1 ms (about 24.8 days) as 1 ms, so larger values must be clamped
+const MAX_TIMEOUT = 2147483647
+
 const activeQueryDeprecationNotice = nodeUtils.deprecate(
   () => {},
   'Client.activeQuery is deprecated and will be removed in pg@9.0'
@@ -165,10 +168,11 @@ class Client extends EventEmitter {
     this._connecting = true
 
     if (this._connectionTimeoutMillis > 0) {
+      const connectionTimeout = Math.min(this._connectionTimeoutMillis, MAX_TIMEOUT)
       this.connectionTimeoutHandle = setTimeout(() => {
         con._ending = true
         con.stream.destroy(new Error('timeout expired'))
-      }, this._connectionTimeoutMillis)
+      }, connectionTimeout)
 
       if (this.connectionTimeoutHandle.unref) {
         this.connectionTimeoutHandle.unref()
@@ -703,6 +707,7 @@ class Client extends EventEmitter {
     if (readTimeout) {
       const queryCallback = query.callback || (() => {})
 
+      const readTimeoutDelay = Math.min(readTimeout, MAX_TIMEOUT)
       const readTimeoutTimer = setTimeout(() => {
         const error = new Error('Query read timeout')
 
@@ -728,7 +733,7 @@ class Client extends EventEmitter {
         }
 
         this._pulseQueryQueue()
-      }, readTimeout)
+      }, readTimeoutDelay)
 
       query.callback = (err, res) => {
         clearTimeout(readTimeoutTimer)

packages/pg/lib/native/client.js

@@ -15,6 +15,9 @@ const ConnectionParameters = require('../connection-parameters')
 
 const NativeQuery = require('./query')
 
+// setTimeout() treats a delay above 2^31-1 ms (about 24.8 days) as 1 ms, so larger values must be clamped
+const MAX_TIMEOUT = 2147483647
+
 const queryQueueLengthDeprecationNotice = nodeUtils.deprecate(
   () => {},
   'Calling client.query() when the client is already executing a query is deprecated and will be removed in pg@9.0. Use async/await or an external async flow control mechanism instead.'
@@ -192,6 +195,7 @@ Client.prototype.query = function (config, values, callback) {
   if (readTimeout) {
     queryCallback = query.callback || (() => {})
 
+    const readTimeoutDelay = Math.min(readTimeout, MAX_TIMEOUT)
     readTimeoutTimer = setTimeout(() => {
       const error = new Error('Query read timeout')
 
@@ -212,7 +216,7 @@ Client.prototype.query = function (config, values, callback) {
       }
 
       this._pulseQueryQueue()
-    }, readTimeout)
+    }, readTimeoutDelay)
 
     query.callback = (err, res) => {
       clearTimeout(readTimeoutTimer)

packages/pg/test/unit/client/query-timeout-tests.js

@@ -32,3 +32,53 @@ test('query timeout with Submittable with callback delivers error via callback',
     done()
   })
 })
+
+// setTimeout() treats a delay above 2^31-1 ms as 1 ms, so the client must clamp the delays it passes
+const MAX_TIMEOUT = 2147483647
+
+// Record the delay of every setTimeout call made by `fn`, without scheduling anything
+const recordTimerDelays = (fn) => {
+  const originalSetTimeout = global.setTimeout
+  const delays = []
+  global.setTimeout = (cb, delay) => {
+    delays.push(delay)
+    return { unref() {} }
+  }
+  try {
+    fn()
+  } finally {
+    global.setTimeout = originalSetTimeout
+  }
+  return delays
+}
+
+test('query_timeout above 2^31-1 ms is clamped to what setTimeout supports', function () {
+  const client = helper.client()
+  client.connectionParameters = { query_timeout: Math.pow(2, 31) }
+  client.connection.emit('readyForQuery')
+
+  const delays = recordTimerDelays(() => client.query('SELECT 1', () => {}))
+  assert.deepEqual(delays, [MAX_TIMEOUT])
+})
+
+test('per-query query_timeout above 2^31-1 ms is clamped to what setTimeout supports', function () {
+  const client = helper.client()
+  client.connection.emit('readyForQuery')
+
+  const delays = recordTimerDelays(() => client.query({ text: 'SELECT 1', query_timeout: Infinity }, () => {}))
+  assert.deepEqual(delays, [MAX_TIMEOUT])
+})
+
+test('query_timeout within the supported range is passed to setTimeout unchanged', function () {
+  const client = helper.client()
+  client.connectionParameters = { query_timeout: 60000 }
+  client.connection.emit('readyForQuery')
+
+  const delays = recordTimerDelays(() => client.query('SELECT 1', () => {}))
+  assert.deepEqual(delays, [60000])
+})
+
+test('connectionTimeoutMillis above 2^31-1 ms is clamped to what setTimeout supports', function () {
+  const delays = recordTimerDelays(() => helper.client({ connectionTimeoutMillis: Math.pow(2, 31) }))
+  assert.deepEqual(delays, [MAX_TIMEOUT])
+})