Proposal of another poolSettings parameter

[pisabev]

Currently connection leak detection can destroy a connection in busy state if the query exceeds the "leakDetectionThreshold" timeout. I have tried this code in place of "idleTimeout" property just to check if it works. It will destroy connection only if the connection state is in "Idle" state and not released to the pool. Seems ideal for leaking connections. Or I'm missing something ?

_checkIdleTimeout(PooledConnectionImpl pconn) {
if (_connections.length > settings.minConnections) {
if (pconn._state != available //Not available
&& pconn._connection.state == idle //But in idle state
&& pconn._obtained != null
&& _isExpired(pconn._obtained, settings.idleTimeout)) {
_debug('Idle connection ${pconn.name}.');
_destroyConnection(pconn);
}
}
}

[xxgreg]

From memory I believe I copied this behaviour from Hikari https://github.com/brettwooldridge/HikariCP . But it has been a while since I wrote it - so my memory is a bit hazy about the specifics.

I believe the idea is that the pool can also clean up deadlocked connections, or connections which are taking too long to perform a query.

I'm open to adding settings for this - but perhaps it's worth first having a look at common Hikari production settings to get some inspiration.

[pisabev]

After some investigation of the HikariCP driver I found these comments quite interesting:

https://groups.google.com/forum/#!topic/hikari-cp/zBKo96xoHXI - (the second comment)
and
brettwooldridge/HikariCP#114

So in short they have two main arguments against having option like that. The first applies to threads and being not sure that some other thread is not using the same connection and the second as being bad practice. The first argument seems to not apply here, as for the second - yes it is bad practice and sure we should use that option with a coution, but sometimes it can be lifesafer till the leak is found.

I know see that this can be implemented outside the driver as we have access to "obtained" and "connectionState", so I'm a little confused wether it's a good place for this in the driver, but comparing it with leakDetectionThreshold it doesn't seem more damaging.

[xxgreg]

Thanks for digging into this.

Perhaps the right thing to do is to add a setting called something like :
closeLeakedConnectionsIfBusy

I'm pretty snowed under with work at the moment, but might be able to look
at this during the week.

On Tue, Aug 11, 2015 at 10:16 AM, Petar Sabev [email protected]
wrote:

After some investigation of the HikariCP driver I found these comments
quite interesting:

https://groups.google.com/forum/#!topic/hikari-cp/zBKo96xoHXI - (the
second comment)
and
brettwooldridge/HikariCP#114
brettwooldridge/HikariCP#114

So in short they have two main arguments against having option like that.
The first applies to threads and being not sure that some other thread is
not using the same connection and the second as being bad practice. The
first argument seems to not apply here, as for the second - yes it is bad
practice and sure we should use that option with a coution, but sometimes
it can be lifesafer till the leak is found.

I know see that this can be implemented outside the driver as we have
access to "obtained" and "connectionState", so I'm a little confused wether
it's a good place for this in the driver, but comparing it with
leakDetectionThreshold it doesn't seem more damaging.

—
Reply to this email directly or view it on GitHub
#66 (comment)
.

[artem-malko]

@xxgreg hello, any updates here?)