Connection Pool Reaper #218
Conversation
| @@ -138,6 +147,14 @@ public static ConnectionPool GetPool(ConnectionSettings cs) | |||
| await pool.ClearAsync(ioBehavior, cancellationToken).ConfigureAwait(false); | |||
| } | |||
|
|
|||
| public static async Task ReapPoolsAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) | |||
| { | |||
| var pools = new List<ConnectionPool>(s_pools.Values); | |||
There was a problem hiding this comment.
My understanding is that the Values collection is thread-safe and can be enumerated on one thread while the dictionary is being modified on a different thread. (It will represent a snapshot of the dictionary at the time the property was accessed.)
Thus, the local copy of s_pools.Values is unnecessary and this could be inlined. (Same for the existing ClearPoolsAsync.)
| @@ -147,6 +164,21 @@ private ConnectionPool(ConnectionSettings cs) | |||
| } | |||
|
|
|||
| static readonly ConcurrentDictionary<string, ConnectionPool> s_pools = new ConcurrentDictionary<string, ConnectionPool>(); | |||
| static readonly TimeSpan ReapTimeSpan = TimeSpan.FromMinutes(3); | |||
| static readonly Task Reaper = Task.Run(async () => { | |||
There was a problem hiding this comment.
In times past, I would have created a dedicated background worker thread that used Thread.Sleep. I haven't personally used this particular approach, but I don't see anything wrong with it.
I was wondering if we needed to use a CancellationToken... but there's no mechanism by which it could ever be cancelled; the connection pools live for the lifetime of the process.
(I can imagine an edge case where a consumer finishes using MySQL, calls MySqlConnection.ClearAllPools and would like all background code to stop running, but that seems too unlikely to be worth writing code to support.)
|
I'm on the fence about whether to merge this in without implementing |
|
I've updated the reaper to include a connection string option,
|
docs/content/connection-options.md
Outdated
| </tr> | ||
| <tr> | ||
| <td>Connection Reset, ConnectionReset </td> | ||
| <td>false</td> | ||
| <td>If true, the connection state is reset when it is retrieved from the pool. The default value of false avoids making an additional server round trip when obtaining a connection, but the connection state is not reset.</td> | ||
| </tr> | ||
| <tr> | ||
| <td>Connection Wait Timeout, ConnectionWaitTimeout</td> |
There was a problem hiding this comment.
Would Idle be a better name than Wait?
Oh, I guess this matches the server's wait_timeout variable.
There was a problem hiding this comment.
I did name it after the server's wait_timeout, but ConnectionIdleTimeout is more self-explanitory, I like that better. I'll update it to ConnectionIdleTimeout.
docs/content/connection-options.md
Outdated
| </tr> | ||
| <tr> | ||
| <td>Connection Reset, ConnectionReset </td> | ||
| <td>false</td> | ||
| <td>If true, the connection state is reset when it is retrieved from the pool. The default value of false avoids making an additional server round trip when obtaining a connection, but the connection state is not reset.</td> | ||
| </tr> | ||
| <tr> | ||
| <td>Connection Wait Timeout, ConnectionWaitTimeout</td> | ||
| <td>180</td> | ||
| <td>The amount of time in seconds that a connection can remain unused in the pool. Any connection that is unused for longer is subject to being closed by a background task runs every minute, unless there are only MinimumPoolSize connections left in the pool. A value of zero (0) means pooled connections will never incur a ConnectionWaitTimeout.</td> |
I don't have any objection to this. (I've heard the advice in the past--don't remember where I read it--to start with It seems like if you set the size of your The reaping process might hold the lock for a long time, but that would happen infrequently. |
|
Trying it with |
|
This implementation is working nicely; I run 1000 connections in 1s in the performance test to load the pool up. Then I run a steady 100rps after that and only a few connections get touched: Eventually, everything that's unused gets reaped: Now to figure out how to write a unit test for this. The reaper is hardcoded to run once per minute, any suggestions on how to write a test for this without waiting a full minute? |
|
The logic in the |
| } | ||
|
|
||
| static readonly ConcurrentDictionary<string, ConnectionPool> s_pools = new ConcurrentDictionary<string, ConnectionPool>(); | ||
| #if DEBUG | ||
| static readonly TimeSpan ReaperInterval = TimeSpan.FromSeconds(1); |
There was a problem hiding this comment.
I added a conditional compilation flag for DEBUG to adjust timing
|
|
||
| namespace SideBySide | ||
| { | ||
| public class DebugOnlyTests : IClassFixture<DatabaseFixture> |
There was a problem hiding this comment.
The SideBySide.DebugOnlyTests class is only compiled when running with "Debug" configuration
@bgrainger this is ready for a final review when you get a chance. Thanks! |
|
Shipped in 0.17.0. |
Min Pool Sizeonce we support thatPlease provide feedback and I can update this PR to something more formal over the next few days. Let me know if there's a "more accepted/efficient" way to create long-running jobs than just spinning up an async task. Also open for ideas on how to better monitor this job to ensure that it doesn't hang.