-
Notifications
You must be signed in to change notification settings - Fork 19
Description
Hi everyone,
I have experienced dropped db connections a couple of times and started searching for a solution. In my case, the connections were dropped by the database server (or maybe due to internet connectivity), and the client was not aware. The default validate()
implementation for mysql checks a couple of flags on the client side, which did not solve the issue as the client did not know about the situation.
I wanted to override validate()
, but failed to use it to my need because it did not accept an async (Promise of bool) function. For example, in JDBC world, a common way to validate a connection before acquiring it from the pool is to send "SELECT 1" query to the db. Later, JDBC came up with a functional interface for this need, which enabled each driver to implement its own default validate function, but still devs can override it to their liking. In this "sequelize-pool" implementation, validate()
function needs to return a sync result, which prevents us all from writing any solution that would send anything over the wire. Without this, I have a bunch of dead connections sitting in the pool, which result in ECONNRESET and 500 error codes on the user side.
As a workaround for my problem, I implemented connection recycling that invalidates connections after a certain maxLifetime
. I commented with my implementation on #51 , you can check it out. However, I now think that this behaves very similarly to setting min
to 0, because if all the min
number of connections sitting idle in the pool have passed their maxLifetime
, the first time you wish to acquire a new connection, you basically have 0 valid connections :) And, this defeats the purpose of wishing to have some healthy minimal number of connections in the pool, in order not to have cold-start kind of situations for requests that arrive after some period of inactivity. Also, this workaround does not fully solve the problem, because some connections may drop before their maxLifetime and still result in errors.
For the reasons I explained above, I believe that making validate()
async is a necessity. If you want to see if the pipe is working, you simply send some water through it :) I see that simply changing validate()
to an async function property might be a breaking change, but this is the road that needs to be taken IMHO. If breaking changes must be avoided at all costs in this library, we can discuss if there is a way to handle this without breaking the current behaviour.