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.

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

Custom keepalive on idle connections protected due to minimum pool size #55

Open
brandonw opened this issue May 10, 2024 · 3 comments
Open

Comments

@brandonw
Copy link

brandonw commented May 10, 2024

Greetings!

While diagnosing some db issues, we realized there may be a feature that could help mitigate or remove aborted connections.

The current state of our setup is:

  • a pool with a min size
  • pool resource idle timeout of 10s
  • db connection idle timeout of 60s

On the surface, one would think there should never be any aborted clients because the pool idle timeout is lower than the db connection idle timeout. However, this is not the case because the pool (rightly) does not reap idle connections once you have reaped down to the pool minimum size.

This means that in periods of low activity, the db will end up aborting the pool idle connections because they are not being used but the pool is protecting them from reaping.

With a goal of reducing the number of db aborted clients to zero, I wonder if it is possible to allow some configuration in [1]. The goal being something like:

  1. removeIdle triggers
  2. all idle connections down to min pool size are reaped as normal
  3. all idle connections below the min pool size allow a config option to be called with that resource to basically tell the resource "do what you need to do in order to keep yourself healthy"

In my specific case using mysql2, this would probably be something like pooledObject.resource.ping().

The end result would be that even in periods of low traffic, the pooled connections below the min threshold would keep themselves healthy by continually resetting the idle timeout on the server so that they are not closed due to lack of usage.

Would a feature like that be useful or accepted? If so, I may be able to work on a PR.

[1]

sequelize-pool/src/Pool.ts

Lines 263 to 277 in da269d7

// Go through the available (idle) items,
// check if they have timed out
for (i = 0; i < available && maxRemovable > toRemove.length; i++) {
timeout = this._availableObjects[i].timeout;
if (now >= timeout) {
// Client timed out, so destroy it.
this._log(
'removeIdle() destroying obj - now:' + now + ' timeout:' + timeout,
'verbose',
);
toRemove.push(this._availableObjects[i].resource);
}
}
toRemove.forEach(this.destroy, this);

@brandonw
Copy link
Author

An added difficulty here would be that the operation of telling the server that you are healthy likely means you have to effectively "acquire" the resource so that you can do whatever the resource needs to do to remain healthy. Otherwise, the ping() could be mid-execution while another client tries to acquire that resource while it is technically not usable.

@sushantdhiman
Copy link
Collaborator

Another alternative for keeping the min pooled resources healthy is to use time based maxUses, as discussed in #51. Will such an implementation suit your needs?

@brandonw
Copy link
Author

In my specific case, I don't think that would help, since in this context there are no uses at all, so the resource would still be maintained while idle and not recycled.

I am trying out a min pool size of zero and seeing if there is not any adverse effect on request latency. Realistically, the spikes we get are not very short where having a min pool size would be most useful. They are relatively longer lived (at least into the minute or more than a minute) where there will be enough time to ramp up the connection pool and persist all the resources as they will be in constant use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants