Bug fix: corrected sleep time calculation in IdleConnectionEvictor #789
Conversation
…se 1 minute sleep time by default
|
What was the actual bug here? I'm staring at the code and I can't spot it. If there's a bug in sleep time calculation here, then there's a very high chance I hit it in production not two weeks ago and failed to realize it (I thought I just misconfigured the client). |
| localSleepTime.sleep(); | ||
| connectionManager.closeExpired(); | ||
| if (maxIdleTime != null) { | ||
| connectionManager.closeIdle(maxIdleTime); |
There was a problem hiding this comment.
I have a suggestion here. An issue with this code is that it never evicts connections until they've already been sitting expired in the pool, which creates a window of opportunity for stale connection reuse.
Instead of passing in maxIdleTime, I would pass in maxIdleTime - localSleepTime. This will cause all connections to be closed that will exceed their maxIdleTime during the next "tick" (where localSleepTime is the duration of a tick). This preserves the invariant that the conn pool never leases a connection that has been idle longer than maxIdleTime.
There was a problem hiding this comment.
Another option is to rationalize evictIdleConnections and ConnectionConfig.idleTimeout in the client builders. The former could be used as the default value for the latter. This would provide a much stronger guarantee than my previous suggestion, which is still prone to race conditions since we don't have real-time guarantees about when IdleConnectionEvictor runs.
2 participants
IdleConnectionEvictormaxIdleTimehas not been specified