change pool.end behavior, add force argument [breaking change, v9?]

[ghost]

change pool.end([cb]) behavior, add force argument

• process pending queries by default
• allow optional force argument to drop pending queries

pool.end([force][, cb])

  it('pool.end() - finish pending queries by default', async () => {
    const pool = new Pool({ poolSize: 10 }) // pool size 10
    let completed = 0
    for (let x = 1; x <= 20; x++) { // queue up 20 queries
       pool.query('SELECT $1::text as name', ['brianc']).then(() => completed++)
    }
    await pool.end() // pool.end()
    expect(completed).to.equal(20) // all 20 queries finish
  })

  it('pool.end(true) - drop pending queries', async () => {
    const pool = new Pool({ poolSize: 10 }) // pool size 10
    let completed = 0
    for (let x = 1; x <= 20; x++) { // queue up 20 queries
       pool.query('SELECT $1::text as name', ['brianc']).then(() => completed++)
    }
    await pool.end(true) // pool.end(true)
    expect(completed).to.equal(10) // 10 active queries finish, 10 pending queries get dropped
  })

[brianc]

Thanks for the idea - I'm not going to be able to merge this unless you can write some pretty comprehensive tests covering the feature as well - anything I merge I'm responsible for maintaining indefinitely and without tests that burden is unbearable!

[brianc]

@brianc I tried running the existing tests on master branch for pg-pool and it is throwing require errors so Im not sure the existing tests are even working, pg-pool/test/ending.js is a single commit from 3 years ago?

the tests run in CI on every commit...I am preeeetty sure they're working. What commands exactly did you run & what was the output?

[ghost]

Ok I see in your Readme to use yarn test

[ghost]

@brianc I am trying to write better test for you here but I am running into this bug I mentioned earlier brianc/node-pg-pool#134 it is throwing off my test to have pool.end complete before the last pool.query, any plans on fixing that?

[ghost]

@brianc ok I wrote better tests. Only problem is that bug #2163 is causing this line to be 19 instead of 20 https://github.com/brianc/node-postgres/pull/2149/files#diff-57867471f03f2f65dd6fde482c6f9ea2R48 and this line to be 9 instead of 10 https://github.com/brianc/node-postgres/pull/2149/files#diff-57867471f03f2f65dd6fde482c6f9ea2R58

fix that bug then I will update this to 10 and 20 instead of 9 and 19

[ghost]

Anyone have any thoughts on this PR? Anyone else think this should be the default behavior or is it just me? Also is this issue #2163 considered a bug that needs to be fixed? there are no comments on this PR or issue @brianc @charmander

[charmander]

Sure, I think it makes sense for this to be the default behaviour. Is force even necessary? I’m not sure when it would be the best solution to a problem.

[ghost]

I don't think I would ever use force but best to give the option of dropping pending queries to preserve the same functionality as previous pool.end for those that want it. Anyone feel free to update this PR with any changes

[ghost]

does this line also need to be updated? by changing this.ending to (this.ending && (!this._pendingQueue.length || this.force))

node-postgres/packages/pg-pool/index.js

Line 300 in 2bd8fe8

if (err || this.ending || !client._queryable || client._ending || client._poolUseCount >= this.options.maxUses) {

[charmander]

Since the author has deleted their account and can’t update the PR anymore, I’ll propose a version of this in a new one.