Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add connection lifetime limit option and tests #2698

Merged
merged 1 commit into from Jan 28, 2022
Merged

Add connection lifetime limit option and tests #2698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/pg-pool/index.js
View file
Open in desktop
Expand Up @@ -84,6 +84,7 @@ class Pool extends EventEmitter {
this.options.max = this.options.max || this.options.poolSize || 10
this.options.maxUses = this.options.maxUses || Infinity
this.options.allowExitOnIdle = this.options.allowExitOnIdle || false
this.options.maxLifetimeSeconds = this.options.maxLifetimeSeconds || 0
this.log = this.options.log || function () {}
this.Client = this.options.Client || Client || require('pg').Client
this.Promise = this.options.Promise || global.Promise
Expand All @@ -94,6 +95,7 @@ class Pool extends EventEmitter {

this._clients = []
this._idle = []
this._expired = new WeakSet()
this._pendingQueue = []
this._endCallback = undefined
this.ending = false
Expand Down Expand Up @@ -123,6 +125,7 @@ class Pool extends EventEmitter {
}
return
}

// if we don't have any waiting, do nothing
if (!this._pendingQueue.length) {
this.log('no queued requests')
Expand Down Expand Up @@ -248,6 +251,17 @@ class Pool extends EventEmitter {
} else {
this.log('new client connected')

if (this.options.maxLifetimeSeconds !== 0) {
setTimeout(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of missing unref, this effectively disables allowExitOnIdle feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the unit test need to be updated? The test passed.

✓ unrefs the connections and timeouts so the program can exit when idle when the allowExitOnIdle option is set (151ms)

Copy link

@polomsky polomsky Feb 11, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. maxLifetimeSeconds should be set there to some higher value, e.g. 60.

Copy link

@hempels hempels Feb 15, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think an additional test is needed in idle-timeout.js since allowExitOnIdle should be tested under both conditions (maxLifetimeSeconds is disabled and maxLifetimeSeconds is a longer time period than idleTimeoutMillis.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to also comment on this, but this is preventing a clean shutdown even when client.end() is called.
Can't we just use .unref() on the timeout or

const timeout = setTimeout(....)
client.once('end', () => clearTimeout(timeout))

Maybe even both

Edit: Something like that

          const maxLifetimeTid = setTimeout(() => {
            this.log('ending client due to expired lifetime')
            this._expired.add(client)
            const idleIndex = this._idle.findIndex((idleItem) => idleItem.client === client)
            if (idleIndex !== -1) {
              this._acquireClient(
                client,
                new PendingItem((err, client, clientRelease) => clientRelease()),
                idleListener,
                false
              )
            }
          }, this.options.maxLifetimeSeconds * 1000)

          client.once('end', () => clearTimeout(maxLifetimeTid))

          if (this.options.allowExitOnIdle) {
            maxLifetimeTid.unref()
          }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth noting that the default setting for maxLifetimeSeconds is 0 (aka: disabled) and none of these issues appear unless you set it to a non-zero value. That doesn't resolve the issues, but it is important to be aware of since there is a simple workaround (set it back to 0.) It seems like the need to utilize maxLifetimeSeconds would be minimal in cases where clients are being explicitly disconnected (the feature was added primarily to support long-running services in load-balanced DB environments.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly my use case (for faster recovery after a failover to a replica) 😊
I just ran into some issue with some local scripts, we're I'm using the same config. I might do a PR to fix it in the next few days, unless anybody else is on it. But haven't seen any PRs.

this.log('ending client due to expired lifetime')
this._expired.add(client)
const idleIndex = this._idle.findIndex(idleItem => idleItem.client === client)
if (idleIndex !== -1) {
this._acquireClient(client, new PendingItem((err, client, clientRelease) => clientRelease()), idleListener, false)
}
}, this.options.maxLifetimeSeconds * 1000)
}

return this._acquireClient(client, pendingItem, idleListener, true)
}
})
Expand Down Expand Up @@ -318,6 +332,15 @@ class Pool extends EventEmitter {
return
}

const isExpired = this._expired.has(client)
if (isExpired) {
this.log('remove expired client')
this._expired.delete(client)
this._remove(client)
this._pulseQueue()
return
}

// idle timeout
let tid
if (this.options.idleTimeoutMillis) {
Expand Down Expand Up @@ -414,6 +437,10 @@ class Pool extends EventEmitter {
return this._idle.length
}

get expiredCount() {
return this._clients.reduce((acc, client) => acc + (this._expired.has(client) ? 1 : 0), 0)
}

get totalCount() {
return this._clients.length
}
Expand Down
46 changes: 46 additions & 0 deletions packages/pg-pool/test/lifetime-timeout.js
View file
Open in desktop
@@ -0,0 +1,46 @@
'use strict'
const co = require('co')
const expect = require('expect.js')

const describe = require('mocha').describe
const it = require('mocha').it
const path = require('path')

const Pool = require('../')

describe('lifetime timeout', () => {
it('connection lifetime should expire and remove the client', (done) => {
const pool = new Pool({ maxLifetimeSeconds: 1 })
pool.query('SELECT NOW()')
pool.on('remove', () => {
console.log('expired while idle - on-remove event')
expect(pool.expiredCount).to.equal(0)
expect(pool.totalCount).to.equal(0)
done()
})
})
it('connection lifetime should expire and remove the client after the client is done working', (done) => {
const pool = new Pool({ maxLifetimeSeconds: 1 })
pool.query('SELECT pg_sleep(1.01)')
pool.on('remove', () => {
console.log('expired while busy - on-remove event')
expect(pool.expiredCount).to.equal(0)
expect(pool.totalCount).to.equal(0)
done()
})
})
it('can remove expired clients and recreate them',
co.wrap(function* () {
const pool = new Pool({ maxLifetimeSeconds: 1 })
let query = pool.query('SELECT pg_sleep(1)')
expect(pool.expiredCount).to.equal(0)
expect(pool.totalCount).to.equal(1)
yield query
expect(pool.expiredCount).to.equal(0)
expect(pool.totalCount).to.equal(0)
yield pool.query('SELECT NOW()')
expect(pool.expiredCount).to.equal(0)
expect(pool.totalCount).to.equal(1)
})
)
})