Ayende @ Rahien

It's a girl

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

Tommy Carlier
06/06/2009 05:36 PM by
Tommy Carlier

PooledDistributedHashTableStorageClientConnection? Try discussing that class on Twitter. :-P

SHODAN
06/06/2009 05:56 PM by
SHODAN

Oh, the joys of naming guidelines gone berserk.

Marc Brooks
06/06/2009 07:14 PM by
Marc Brooks

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.

Stephen
06/06/2009 09:14 PM by
Stephen

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.

Ayende Rahien
06/06/2009 10:00 PM by
Ayende Rahien

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

Ayende Rahien
06/06/2009 10:08 PM by
Ayende Rahien

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

Andrew
06/07/2009 04:46 AM by
Andrew

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.

Ayende Rahien
06/07/2009 04:58 AM by
Ayende Rahien

Andrew,

That would require me to implement a reset logic, something which would probably be quite complicated.

Comments have been closed on this topic.