From d3aee3dfc8b698358eaf76587fd95f1f745494a0 Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Mon, 28 Oct 2019 11:45:20 -0500 Subject: [PATCH 1/2] Add additional pool test & deprecate .end There are no tests covering cursor.end(). It's also a weird method to have on the cursor as it terminates the connection to postgres internally. I don't recall why I added this method in the first place but its not correct to close a connection to postgres by calling .end on a cursor...seems like a pretty big footgun. If you have a pooled client and you call `cursor.end` it'll close the client internally and likely confuse the pool. Plus it's just weird to be able to close a connection by calling .end on a query or cursor. So...I'm deprecating that method. --- index.js | 6 ++++-- test/pool.js | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 8136c389..a6960081 100644 --- a/index.js +++ b/index.js @@ -154,13 +154,15 @@ Cursor.prototype._getRows = function(rows, cb) { this.connection.flush() } -Cursor.prototype.end = function(cb) { +// users really shouldn't be calling 'end' here and terminating a connection to postgres +// via the low level connection.end api +Cursor.prototype.end = util.deprecate(function(cb) { if (this.state !== 'initialized') { this.connection.sync() } this.connection.once('end', cb) this.connection.end() -} +}, 'Cursor.end is deprecated. Call end on the client itself to end a connection to the database.') Cursor.prototype.close = function(cb) { if (this.state === 'done') { diff --git a/test/pool.js b/test/pool.js index bd487807..80e0ddc1 100644 --- a/test/pool.js +++ b/test/pool.js @@ -83,4 +83,24 @@ describe('pool', function() { done() }) }) + + it('can close multiple times on a pool', async function() { + const pool = new pg.Pool({ max: 1 }) + const run = () => + new Promise(async resolve => { + const cursor = new Cursor(text) + const client = await pool.connect() + client.query(cursor) + cursor.read(25, function(err) { + assert.ifError(err) + cursor.close(function(err) { + assert.ifError(err) + client.release() + resolve() + }) + }) + }) + await Promise.all([run(), run(), run()]) + await pool.end() + }) }) From cedce4bded8cf4970de6ad95e62c411d639cd51e Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Wed, 30 Oct 2019 12:52:06 -0500 Subject: [PATCH 2/2] Fix lint & enable all tests --- test/pool.js | 11 ++++++----- test/transactions.js | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/test/pool.js b/test/pool.js index 80e0ddc1..61f5e279 100644 --- a/test/pool.js +++ b/test/pool.js @@ -86,11 +86,11 @@ describe('pool', function() { it('can close multiple times on a pool', async function() { const pool = new pg.Pool({ max: 1 }) - const run = () => - new Promise(async resolve => { - const cursor = new Cursor(text) - const client = await pool.connect() - client.query(cursor) + const run = async () => { + const cursor = new Cursor(text) + const client = await pool.connect() + client.query(cursor) + new Promise(resolve => { cursor.read(25, function(err) { assert.ifError(err) cursor.close(function(err) { @@ -100,6 +100,7 @@ describe('pool', function() { }) }) }) + } await Promise.all([run(), run(), run()]) await pool.end() }) diff --git a/test/transactions.js b/test/transactions.js index 37af28c4..f83cc1d7 100644 --- a/test/transactions.js +++ b/test/transactions.js @@ -28,7 +28,7 @@ describe('transactions', () => { await client.end() }) - it.only('can execute multiple statements in a transaction if no data', async () => { + it('can execute multiple statements in a transaction if no data', async () => { const client = new pg.Client() await client.connect() await client.query('begin')