Connection Pooling: Implemention
Given the following contact:
/// <summary> /// Thread Safety - This is NOT a thread safe connection /// Exception Safety - After an exception is thrown, it should be disposed and not used afterward /// Connection Pooling - It is expected that this will be part of a connection pool /// </summary> public class DistributedHashTableStorageClient
I decided that I needed to really didn’t want to pass the responsibility for that to the client, and that I wanted to handle that inside my library. Here is what I came up with:
public class DefaultConnectionPool { private static readonly ILog log = LogManager.GetLogger(typeof (DefaultConnectionPool)); readonly object locker = new object(); private readonly Dictionary<NodeEndpoint, LinkedList<PooledDistributedHashTableStorageClientConnection>> pooledConnections = new Dictionary<NodeEndpoint, LinkedList<PooledDistributedHashTableStorageClientConnection>>(); public IDistributedHashTableStorage Create(NodeEndpoint endpoint) { PooledDistributedHashTableStorageClientConnection storage = null; lock (locker) { LinkedList<PooledDistributedHashTableStorageClientConnection> value; if (pooledConnections.TryGetValue(endpoint, out value) && value.Count > 0) { storage = value.First.Value; value.RemoveFirst(); } } if (storage != null) { if (storage.Connected == false) { log.DebugFormat("Found unconnected connection in the pool for {0}", endpoint.Sync); try { storage.Dispose(); } catch (Exception e) { log.Debug("Error when disposing unconnected connection in the pool", e); } } else { return storage; } } log.DebugFormat("Creating new connection in the pool to {0}", endpoint.Sync); return new PooledDistributedHashTableStorageClientConnection(this, endpoint); } private void PutMeBack(PooledDistributedHashTableStorageClientConnection connection) { lock (locker) { LinkedList<PooledDistributedHashTableStorageClientConnection> value; if (pooledConnections.TryGetValue(connection.Endpoint, out value) == false) { pooledConnections[connection.Endpoint] = value = new LinkedList<PooledDistributedHashTableStorageClientConnection>(); } value.AddLast(connection); } log.DebugFormat("Put connection for {0} back in the pool", connection.Endpoint.Sync); } class PooledDistributedHashTableStorageClientConnection : DistributedHashTableStorageClient { private readonly DefaultConnectionPool pool; public PooledDistributedHashTableStorageClientConnection( DefaultConnectionPool pool, NodeEndpoint endpoint) : base(endpoint) { this.pool = pool; } public bool Connected { get { return client.Connected; } } public override void Dispose() { if(Marshal.GetExceptionCode() != 0)//we are here because of some sort of error { log.Debug("There was an error during the usage of pooled client connection, will not return it to the pool (may be poisioned)"); base.Dispose(); } else if(Connected == false) { log.Debug("The connection was disconnected, will not return connection to the pool"); base.Dispose(); } else { pool.PutMeBack(this); } } } }
I think that should pretty much cover everything I need.
Thoughts?
Comments
PooledDistributedHashTableStorageClientConnection? Try discussing that class on Twitter. :-P
Oh, the joys of naming guidelines gone berserk.
I'm uncomfortable with the incestuous probing in Dispose for the Marshal.GetExceptionCode. Seems very fragile to have that implementation logic dependency. Would it be better to have a delegate wrapper that allows you to catch the actual exception that would have resulted and flag and handle it safely without that assumption?
My other, more vague, concern is the placing of the "partially setup" LinkedList in PutMeBack. It seems that you shouldn't be shoving an empty LinkedList into the pooled connection hashtable until you have added the connection to the list.
One option would be to set a boolean flag based on the TryGetValue failure, new up the LinkedList, and then fall through to the AddLast. Then, after the list is now fully valid, check the flag and shove the new list in the pooled connections if needed.
Better yet, construct if the TryGetValue failed, new the LinkedList, AddLast the connection and then shove the new list in the pooled connections. Then the else-clause of the TryGetValue is merely responsible for an AddLast call against an already-existing list.
Last concern, I'm troubled by the "hungarianish" naming of your connection hashtable. Seems like an awful lot of implementation details being called out in the name of the class... Unless you have tons of other kinds running around, I would suggest PooledClientConnection or similar intention-revealing (instead of implementation-declaring) naming.
By 'an exception being thrown'.. this would be similar to WCF where a client instance is dumped once its state is error'd? in which case it seems sensible that the connection should indicate if it is in an error'd state.. rather than odd tricks trying to find out if it is in error'd state or not.
Marc,
I agree that the Marshal trick isn't nice, but it is the only alternative to the delegate trick, and I am getting tired of recursive delegates tricks.
Partially setup? I am not sure that I understand. It is perfectly legit to have an empty list in the pool.
What is the implementation about the class name?It is implementing DistributedHashTableStorageClient, and the rest is just what it _is_.
Client has no meaning in the app
Stephen,
No, the idea is that if I am getting an error when using this, I am not sure what the state of the server is, so I am dumping the connection.
There is no easy way for me to discover that, so an error will trigger the dumping.
And yes, I could have done this differently, but that would mean that I would have to:
a) modify the base class
b) do more work
This is actually simpler than all the alternatives, and it produces a "don't need to think about it" semantics for the user
Repository (as in database) pooling logic (such as MySQL .Net Connector, and MS SQL's connector) keep a pool of connections too.
Prior to handing a connection to the client (or when its put back, I forget which), the connection is 'reset', that clears all session state etc information to make sure the pooled connection being returned is 'fresh'.
If an SQL error occurs when someone uses a connection, it can still be disposed of and put back in the pool - because its reset when given to another client.
That could be an alternative to deciding whether to re-pool the connection based upon whether the previous disposal was because of an error.
Andrew,
That would require me to implement a reset logic, something which would probably be quite complicated.
Comment preview