From 4bc19e69e86eb2fa196328ce539721c2362a33b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20=C5=BDaromskis?= Date: Mon, 27 Aug 2018 17:38:54 +0300 Subject: [PATCH 01/11] Add read_timeout to connection settings --- lib/client.js | 30 ++++++++++++++++++++++++++++++ lib/connection-parameters.js | 1 + lib/defaults.js | 5 ++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 625aad682..873e99876 100644 --- a/lib/client.js +++ b/lib/client.js @@ -403,6 +403,10 @@ Client.prototype.query = function (config, values, callback) { if (config === null || config === undefined) { throw new TypeError('Client was passed a null or undefined query') } else if (typeof config.submit === 'function') { + var readTimeoutTimer + var queryCallback + var readTimeout = config.query_timeout || this.connectionParameters.query_timeout + result = query = config if (typeof values === 'function') { query.callback = query.callback || values @@ -416,6 +420,32 @@ Client.prototype.query = function (config, values, callback) { } } + if (readTimeout) { + queryCallback = query.callback; + + readTimeoutTimer = setTimeout(() => { + var error = new Error('Query read timeout'); + + this.emit('error', error) + queryCallback(error) + + // we already returned an error, + // just do nothing when query completes + delete query.callback + + // Remove from queue + var index = this.queryQueue.indexOf(query) + if (index > -1) { + this.queryQueue.splice(index, 1) + } + }, readTimeout) + + query.callback = (err, res) => { + clearTimeout(readTimeoutTimer) + queryCallback(err, res) + } + } + if (this.binary && !query.binary) { query.binary = true } diff --git a/lib/connection-parameters.js b/lib/connection-parameters.js index 19267058f..745311ad0 100644 --- a/lib/connection-parameters.js +++ b/lib/connection-parameters.js @@ -65,6 +65,7 @@ var ConnectionParameters = function (config) { this.application_name = val('application_name', config, 'PGAPPNAME') this.fallback_application_name = val('fallback_application_name', config, false) this.statement_timeout = val('statement_timeout', config, false) + this.query_timeout = val('query_timeout', config, false) } // Convert arg to a string, surround in single quotes, and escape single quotes and backslashes diff --git a/lib/defaults.js b/lib/defaults.js index 30214b1d8..6b0a98f1b 100644 --- a/lib/defaults.js +++ b/lib/defaults.js @@ -55,7 +55,10 @@ module.exports = { parseInputDatesAsUTC: false, // max milliseconds any query using this connection will execute for before timing out in error. false=unlimited - statement_timeout: false + statement_timeout: false, + + // max miliseconds to wait for query to complete (client side) + query_timeout: false } var pgTypes = require('pg-types') From 27bf328eb443c865092cb96c54e7d8578c53623a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20=C5=BDaromskis?= Date: Tue, 28 Aug 2018 11:43:13 +0300 Subject: [PATCH 02/11] Fix uncaught error issue --- lib/client.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/client.js b/lib/client.js index 873e99876..3d0fb67fe 100644 --- a/lib/client.js +++ b/lib/client.js @@ -424,20 +424,22 @@ Client.prototype.query = function (config, values, callback) { queryCallback = query.callback; readTimeoutTimer = setTimeout(() => { - var error = new Error('Query read timeout'); + var error = new Error('Query read timeout') this.emit('error', error) queryCallback(error) // we already returned an error, - // just do nothing when query completes - delete query.callback + // just do nothing if query completes + query.callback = () => {} // Remove from queue var index = this.queryQueue.indexOf(query) if (index > -1) { this.queryQueue.splice(index, 1) } + + this._pulseQueryQueue() }, readTimeout) query.callback = (err, res) => { From 6ae44d50747dffe12ca4cc478ce935c94a11b5bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20=C5=BDaromskis?= Date: Tue, 28 Aug 2018 14:50:44 +0300 Subject: [PATCH 03/11] Fix lint --- lib/client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 3d0fb67fe..8eb9351b1 100644 --- a/lib/client.js +++ b/lib/client.js @@ -421,7 +421,7 @@ Client.prototype.query = function (config, values, callback) { } if (readTimeout) { - queryCallback = query.callback; + queryCallback = query.callback readTimeoutTimer = setTimeout(() => { var error = new Error('Query read timeout') From 2196e4c49e27ad78b025a5b9aad1e2378d25647f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20=C5=BDaromskis?= Date: Tue, 28 Aug 2018 16:19:29 +0300 Subject: [PATCH 04/11] Fix "queryCallback is not a function" --- lib/client.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/client.js b/lib/client.js index 8eb9351b1..35cc82d0d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -427,11 +427,15 @@ Client.prototype.query = function (config, values, callback) { var error = new Error('Query read timeout') this.emit('error', error) - queryCallback(error) - // we already returned an error, - // just do nothing if query completes - query.callback = () => {} + if (typeof queryCallback === 'function') { + queryCallback(error) + + // we already returned an error, + // just do nothing if query completes + query.callback = () => { + } + } // Remove from queue var index = this.queryQueue.indexOf(query) From 9798622a8bae6e5afa9d73edf01e9f9dc9fdb112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Fri, 2 Nov 2018 15:25:57 +0000 Subject: [PATCH 05/11] Added test and fixed error returning --- lib/client.js | 13 ++++++++----- test/integration/client/api-tests.js | 11 +++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/lib/client.js b/lib/client.js index 35cc82d0d..642684902 100644 --- a/lib/client.js +++ b/lib/client.js @@ -399,19 +399,20 @@ Client.prototype.query = function (config, values, callback) { // can take in strings, config object or query object var query var result + var readTimeout + var readTimeoutTimer + var queryCallback if (config === null || config === undefined) { throw new TypeError('Client was passed a null or undefined query') } else if (typeof config.submit === 'function') { - var readTimeoutTimer - var queryCallback - var readTimeout = config.query_timeout || this.connectionParameters.query_timeout - + readTimeout = config.query_timeout || this.connectionParameters.query_timeout result = query = config if (typeof values === 'function') { query.callback = query.callback || values } } else { + readTimeout = this.connectionParameters.query_timeout query = new Query(config, values, callback) if (!query.callback) { result = new this._Promise((resolve, reject) => { @@ -426,7 +427,9 @@ Client.prototype.query = function (config, values, callback) { readTimeoutTimer = setTimeout(() => { var error = new Error('Query read timeout') - this.emit('error', error) + process.nextTick(() => { + query.handleError(error, this.connection) + }) if (typeof queryCallback === 'function') { queryCallback(error) diff --git a/test/integration/client/api-tests.js b/test/integration/client/api-tests.js index e18c3749c..678213d07 100644 --- a/test/integration/client/api-tests.js +++ b/test/integration/client/api-tests.js @@ -15,6 +15,17 @@ suite.test('pool callback behavior', done => { }) }) +suite.test('query timeout', (cb) => { + const pool = new pg.Pool({query_timeout: 1000}) + pool.connect().then((client) => { + client.query('SELECT pg_sleep(2)', assert.calls(function (err, result) { + assert(err) + client.release() + pool.end(cb) + })) + }) +}) + suite.test('callback API', done => { const client = new helper.Client() client.query('CREATE TEMP TABLE peep(name text)') From 6010c5ca0e1d6b569c3d10d3a23ffa108e15fe4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Fri, 2 Nov 2018 15:51:17 +0000 Subject: [PATCH 06/11] Added query timeout to native client --- lib/native/client.js | 45 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/lib/native/client.js b/lib/native/client.js index c88bfb12e..b965bf82a 100644 --- a/lib/native/client.js +++ b/lib/native/client.js @@ -146,14 +146,21 @@ Client.prototype.connect = function (callback) { Client.prototype.query = function (config, values, callback) { var query var result - - if (typeof config.submit === 'function') { + var readTimeout + var readTimeoutTimer + var queryCallback + + if (config === null || config === undefined) { + throw new TypeError('Client was passed a null or undefined query') + } else if (typeof config.submit === 'function') { + readTimeout = config.query_timeout || this.connectionParameters.query_timeout result = query = config // accept query(new Query(...), (err, res) => { }) style if (typeof values === 'function') { config.callback = values } } else { + readTimeout = this.connectionParameters.query_timeout query = new NativeQuery(config, values, callback) if (!query.callback) { let resolveOut, rejectOut @@ -165,6 +172,40 @@ Client.prototype.query = function (config, values, callback) { } } + if (readTimeout) { + queryCallback = query.callback + + readTimeoutTimer = setTimeout(() => { + var error = new Error('Query read timeout') + + process.nextTick(() => { + query.handleError(error, this.connection) + }) + + if (typeof queryCallback === 'function') { + queryCallback(error) + + // we already returned an error, + // just do nothing if query completes + query.callback = () => { + } + } + + // Remove from queue + var index = this._queryQueue.indexOf(query) + if (index > -1) { + this._queryQueue.splice(index, 1) + } + + this._pulseQueryQueue() + }, readTimeout) + + query.callback = (err, res) => { + clearTimeout(readTimeoutTimer) + queryCallback(err, res) + } + } + if (!this._queryable) { query.native = this.native process.nextTick(() => { From a793e8e15d4b736b60cdd80784fd40337aa9ecf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Fri, 2 Nov 2018 16:07:45 +0000 Subject: [PATCH 07/11] Added test for timeout not reached --- test/integration/client/api-tests.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/integration/client/api-tests.js b/test/integration/client/api-tests.js index 678213d07..ba36368f4 100644 --- a/test/integration/client/api-tests.js +++ b/test/integration/client/api-tests.js @@ -26,6 +26,17 @@ suite.test('query timeout', (cb) => { }) }) +suite.test('query timeout', (cb) => { + const pool = new pg.Pool({query_timeout: 10000}) + pool.connect().then((client) => { + client.query('SELECT pg_sleep(1)', assert.calls(function (err, result) { + assert(!err) + client.release() + pool.end(cb) + })) + }) +}) + suite.test('callback API', done => { const client = new helper.Client() client.query('CREATE TEMP TABLE peep(name text)') From b041a51bb2ab8827ef1df27931deb34966df2533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Fri, 2 Nov 2018 16:14:10 +0000 Subject: [PATCH 08/11] Ensure error is the correct one Correct test name --- test/integration/client/api-tests.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/client/api-tests.js b/test/integration/client/api-tests.js index ba36368f4..648ad2a7d 100644 --- a/test/integration/client/api-tests.js +++ b/test/integration/client/api-tests.js @@ -20,13 +20,14 @@ suite.test('query timeout', (cb) => { pool.connect().then((client) => { client.query('SELECT pg_sleep(2)', assert.calls(function (err, result) { assert(err) + assert(err.message === 'Query read timeout') client.release() pool.end(cb) })) }) }) -suite.test('query timeout', (cb) => { +suite.test('query no timeout', (cb) => { const pool = new pg.Pool({query_timeout: 10000}) pool.connect().then((client) => { client.query('SELECT pg_sleep(1)', assert.calls(function (err, result) { From b127658fff706c379b9e3bbfc842a496734e216c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Sun, 4 Nov 2018 12:12:07 +0000 Subject: [PATCH 09/11] Removed dubious check --- lib/client.js | 11 ++++------- lib/native/client.js | 11 ++++------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/client.js b/lib/client.js index 642684902..8e7d307f7 100644 --- a/lib/client.js +++ b/lib/client.js @@ -431,14 +431,11 @@ Client.prototype.query = function (config, values, callback) { query.handleError(error, this.connection) }) - if (typeof queryCallback === 'function') { - queryCallback(error) + queryCallback(error) - // we already returned an error, - // just do nothing if query completes - query.callback = () => { - } - } + // we already returned an error, + // just do nothing if query completes + query.callback = () => {} // Remove from queue var index = this.queryQueue.indexOf(query) diff --git a/lib/native/client.js b/lib/native/client.js index b965bf82a..5338f7f10 100644 --- a/lib/native/client.js +++ b/lib/native/client.js @@ -182,14 +182,11 @@ Client.prototype.query = function (config, values, callback) { query.handleError(error, this.connection) }) - if (typeof queryCallback === 'function') { - queryCallback(error) + queryCallback(error) - // we already returned an error, - // just do nothing if query completes - query.callback = () => { - } - } + // we already returned an error, + // just do nothing if query completes + query.callback = () => {} // Remove from queue var index = this._queryQueue.indexOf(query) From 1b18dafaad51a60c38771da51d83b0b82646f3c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Tue, 6 Nov 2018 19:17:12 +0000 Subject: [PATCH 10/11] Added new test --- test/integration/client/api-tests.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/integration/client/api-tests.js b/test/integration/client/api-tests.js index 648ad2a7d..9014209bb 100644 --- a/test/integration/client/api-tests.js +++ b/test/integration/client/api-tests.js @@ -27,6 +27,24 @@ suite.test('query timeout', (cb) => { }) }) +suite.test('query recover from timeout', (cb) => { + const pool = new pg.Pool({query_timeout: 1000}) + pool.connect().then((client) => { + client.query('SELECT pg_sleep(20)', assert.calls(function (err, result) { + assert(err) + assert(err.message === 'Query read timeout') + client.release() + pool.connect().then((client) => { + client.query('SELECT pg_sleep(20)', assert.calls(function (err, result) { + assert(!err) + client.release() + pool.end(cb) + })) + }) + })) + }) +}) + suite.test('query no timeout', (cb) => { const pool = new pg.Pool({query_timeout: 10000}) pool.connect().then((client) => { From 14ffc610a92f0ea317366547b7222d4f0e7d563b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Tue, 6 Nov 2018 19:30:30 +0000 Subject: [PATCH 11/11] Improved test --- test/integration/client/api-tests.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/client/api-tests.js b/test/integration/client/api-tests.js index 9014209bb..c274bbd36 100644 --- a/test/integration/client/api-tests.js +++ b/test/integration/client/api-tests.js @@ -33,11 +33,11 @@ suite.test('query recover from timeout', (cb) => { client.query('SELECT pg_sleep(20)', assert.calls(function (err, result) { assert(err) assert(err.message === 'Query read timeout') - client.release() + client.release(err) pool.connect().then((client) => { - client.query('SELECT pg_sleep(20)', assert.calls(function (err, result) { + client.query('SELECT 1', assert.calls(function (err, result) { assert(!err) - client.release() + client.release(err) pool.end(cb) })) })