diff --git a/packages/pg-pool/index.js b/packages/pg-pool/index.js index ab514fa88..6fa018a13 100644 --- a/packages/pg-pool/index.js +++ b/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 diff --git a/packages/pg-pool/test/timer-overflow.js b/packages/pg-pool/test/timer-overflow.js new file mode 100644 index 000000000..390d08867 --- /dev/null +++ b/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() + }) +}) diff --git a/packages/pg/lib/client.js b/packages/pg/lib/client.js index 2b13c1de7..aa1c2eb67 100644 --- a/packages/pg/lib/client.js +++ b/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) diff --git a/packages/pg/lib/native/client.js b/packages/pg/lib/native/client.js index 9ec3c8c03..75c182435 100644 --- a/packages/pg/lib/native/client.js +++ b/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) diff --git a/packages/pg/test/unit/client/query-timeout-tests.js b/packages/pg/test/unit/client/query-timeout-tests.js index dbf20cf1f..85805c653 100644 --- a/packages/pg/test/unit/client/query-timeout-tests.js +++ b/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]) +})