Add functionality to auto-expire pool connections

[Twipped]

Any connection older than connectionExpiry will be terminated upon release or when next reached in the queue.

I've now run up against an issue in two different companies where long living mysql connections have a chance to become stale, and upon doing so end up triggering an error the next time a query tries to run on them. In both cases we've resolved this by forcing the pool to close every 15 minutes, or by regularly pinging open connections to make sure they haven't gone bad. It would be more ideal if the pool just supported killing off a connection after a certain length of time.

[sidorares]

are maxConnectionLife and connectionExpiry parameters the same as in node-mysql ?

would be good to have this in sync with mysqljs/mysql#962

( also long term plan is to move pool / PoolCluster to https://github.com/mysqljs - we already started reusing sqlstring )

[sidorares]

is connectionExpiry based on creation time or last use / idle time?

[Twipped]

Crap, maxConnectionLife wasn't supposed to be there, that's what I initially called the option and then decided connectionExpiry was more apt, since it doesn't actually kill the connection exactly at that time. I'll fix the PR. connectionExpiry does not exist in node-mysql, but I could certainly open a duplicate PR against it.

I wasn't aware of that open issue on node-mysql, but this is a little bit different approach. This PR counts from the time the connection is created, so there is an actual maximum life for the connection itself. We've found things tend to be a bit more stable when you cycle the connections regularly.

I could implement idle timeout in the same way, but the thing about this is that I'm not setting up timeouts to police the connections, I'm just expiring them the next time they enter or exit the pool. I'm not sure if that implementation would meet the needs of that issue.

[Twipped]

If we're talking about doing this in both libraries, @dougwilson should probably be involved in the conversation.

[Twipped]

@sidorares what would we need to get this merged? Do you just want to see identical functionality merged to node-mysql? This PR would help a lot to address #393

[sushantdhiman]

LGTM

[sidorares]

what would we need to get this merged? Do you just want to see identical functionality merged to node-mysql?

I wanted to start some discussion, but I don't mind introducing new api first. At worst if similar but slightly different functionality is added to node-mysql we'll add alias and/or soft deprecate old one here

With this pr: I'm going to release 1.0.0 (hopefully on Monday) and this will go into 1.1.0 soon

[sidorares]

Looked at the code and I'm a bit confused

Wouldn't it be better to add timer-based "max idle time in the pool"? Or the issue is "even if connection is actively used and not idle, after certain time we just can't use it anymore"? Can you think of underlying reason why it's so? Most people complain about "if it's non used at all it might become dead due to server forcibly close it or network issues" - for this we might want to use heartbeat/ping and/or something like idleTimeout discussed in mysqljs/mysql#962

Also, I think we allow to release connections where there are still commands in the queue. Maybe better to wait until queue is empty (or use .close() instead of .terminate())

[Twipped]

The goal was to ensure that connections are regularly being cycled and not just sitting open forever. This is for idle timeout purposes, yes, but also just to ensure any possible cruft related to the connection gets cleared out. Be it undiscovered bugs in mysql2 or in the database itself. Things are just more stable when the connections are refreshed periodically.

We're also still, even in rc12, experiencing problems with connections going dead in the pool and not getting removed until we try to run a query on it and it fails. Currently we're solving this by regularly pinging all inactive connections on the pool and retrying queries when they fail due to a connection loss.

[sidorares]

would be really good to trace original cause (for bugs on mysql2 side), but I see your point

[vlasky]

I think idleConnectionTimeout PR#2218 for mysqljs/mysql could be a good approach for implementing this functionality in node-mysql2.

[Twipped]

Gonna go ahead and close this, since it's WAY out of date and it looks like node-mysql is already addressing the same concern.