Connection timeout with full connection pool overwrites callback queue

[freitagbr]

I think there's an issue with the logic for handling connection timeouts with a full connection pool.

https://github.com/brianc/node-pg-pool/blob/master/index.js#L164:

// set connection timeout on checking out an existing client
const tid = setTimeout(() => {
    // remove the callback from pending waiters because
    // we're going to call it with a timeout error
    this._pendingQueue = this._pendingQueue.filter(cb => cb === response.callback)
    response.callback(new Error('timeout exceeded when trying to connect'))
}, this.options.connectionTimeoutMillis)

On line 164, the intent is to remove the response.callback from this._pendingQueue, because it's going to be handled right now. However, it seems like it actually removes every callback but response.callback from the queue. Is this right?

[brianc]

Hmm you might be on to something. Im not at my computer now but can look in the morning. Is this causing any reproducible issues for you?

On Fri, Nov 17, 2017 at 4:29 PM Brandon Freitag notifications@github.com wrote:

I think there's an issue with the logic for handling connection timeouts with a full connection pool.

https://github.com/brianc/node-pg-pool/blob/master/index.js#L164:

// set connection timeout on checking out an existing clientconst tid = setTimeout(() => { // remove the callback from pending waiters because // we're going to call it with a timeout error this._pendingQueue = this._pendingQueue.filter(cb => cb === response.callback) response.callback(new Error('timeout exceeded when trying to connect')) }, this.options.connectionTimeoutMillis)

On line 164, the intent is to remove the response.callback from this._pendingQueue, because it's going to be handled right now. However, it seems like it actually removes every callback but response.callback from the queue. Is this right?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/brianc/node-pg-pool/issues/85, or mute the thread https://github.com/notifications/unsubscribe-auth/AADDoX1a7WGE7LO6AqOqe5rSO5KLCzYfks5s3gjVgaJpZM4QitNg .

[freitagbr]

Yeah, I was noticing that every once in a while, all of my connections would start timing out. The timeout issue itself is likely an issue with the database. However, when one of the connections in the pool timed-out, the rest of the connections also timed-out. And once all of the connections in the pool timed-out, all of the connections became zombies, and could not reconnect to the database. My connection pool also had reached the max number of connections (20 in my case).

If I'm reading the code correctly, it seems like this might be the cause. I haven't tried fixing it yet, though. If I can find something that reliably fixes the issue, I will let you know.

[ksmith97]

Any update on merging this fix?

[jgeurts]

@brianc Can you please release a new version to npm with this fix?

[poolik]

Looks like we're hitting this issue as well. @brianc what's holding back this bug fix release?

[abenhamdine]

Hello @brianc, could you release a new version please ?
I hit again this issue and it's very annoying when you have a pool exhausted.

[edevil]

@abenhamdine Thanks for the heads-up. This is indeed worrying. :(

[fzaninotto]

I'm also interested in using this fix in production.

[chamini2]

thanks, me too

[brianc]

I'll release a new version right now. I had the flu last week sorry for missing this.

[brianc]

published pg-pool@2.0.4

[kamarajuPrathi]

We still see this error , Can you let us know whats the reason for the check , we are using pg node-postgres libarary which is using pg-pool , we are getting this errors

[charmander]

@kamarajuPrathi Which error? Are you sure it has the same cause as this? If not, it’s best to open a new issue with full details.