4
\$\begingroup\$

I have created a simple class that maintains a pool (with maximum size) of objects. It acts like a connection pool (that is where the name came from - initially it was used just to maintain a pool of SMTP connections). When an item is retrieved - if the pool is not empty, a value is retrieved from it; if it is empty, then if the maximum number of objects have not been allocated, a new one is created.

The usage as it currently stands is:

/// <summary>
/// A generic object pool for any data. A pool is automatically created based on the data type.
/// If the pool size is currently exceeded then the task will wait until an object is returned from the pool.
/// </summary>
public static class ConnectionPool
{
    /// <summary>
    /// Sets the maximum pool size. This method is *NOT* thread-safe and has to be called before the first item is retrieved from the pool.
    /// </summary>
    public static void SetPoolSize<TValue>(int maximumSize)
        where TValue : class
    {
        ConnectionPoolWrapper<TValue>.SetPoolSize(maximumSize);
    }

    /// <summary>
    /// Retrieves a new item from the pool. If the pool is empty and the maximum number of items are active, the task will delay
    /// until an item is freed. 
    /// </summary>
    /// <param name="initializer">Delegate that is used to create a new item if the pool is empty but maximum number of items have not been reached.</param>
    public static Task<ConnectionPoolWrapper<TValue>> RetrieveAsync<TValue>(Func<TValue> initializer)
        where TValue: class
    {
        return ConnectionPoolWrapper<TValue>.RetrieveFromPoolAsync(initializer);
    }


    /// <summary>
    /// Retrieves a new item from the pool. If the pool is empty and the maximum number of items are active, the task will delay
    /// until an item is freed.
    /// </summary>
    /// <remarks>The default constructor is used to create new objects when needed.</remarks>
    public static Task<ConnectionPoolWrapper<TValue>> RetrieveAsync<TValue>()
        where TValue : class, new()
    {
        return ConnectionPoolWrapper<TValue>.RetrieveFromPoolAsync(() => new TValue());
    }
}

/// <summary>
/// An item in the object pool.
/// </summary>
public sealed class ConnectionPoolWrapper<TValue> : IDisposable
    where TValue : class
{
    /// <summary>
    /// Contains the currently allocated and free objects.
    /// </summary>
    private static System.Collections.Concurrent.ConcurrentStack<TValue> _connectionPool = new System.Collections.Concurrent.ConcurrentStack<TValue>();

    /// <summary>
    /// The locking mechanism that makes sure only certain number of items can be allocated in parallel.
    /// </summary>
    private static volatile System.Threading.SemaphoreSlim _counter = null;

    /// <summary>
    /// The object on which a lock is created while <c>_counter</c> is initialized.
    /// </summary>
    private static object _syncRoot = new object();

    /// <summary>
    /// Sets the maximum pool size. This method is *NOT* thread-safe and has to be called before the first item is retrieved from the pool.
    /// </summary>
    public static void SetPoolSize(int maximumSize)
    {
        if (_counter != null)
            throw new InvalidOperationException("The method can only be called once before the first value is retrieved from the pool.");

        _counter = new System.Threading.SemaphoreSlim(maximumSize);
    }

    /// <summary>
    /// Retrieves a new item from the pool. If the pool is empty and the maximum number of items are active, the task will delay
    /// until an item is freed. 
    /// </summary>
    /// <param name="initializer">Delegate that is used to create a new item if the pool is empty but maximum number of items have not been reached.</param>
    public static async Task<ConnectionPoolWrapper<TValue>> RetrieveFromPoolAsync(Func<TValue> initializer)
    {
        if (initializer == null)
            throw new ArgumentNullException("initializer");

        if (_counter == null)
            lock (_syncRoot)
                if (_counter == null)
                    _counter = new System.Threading.SemaphoreSlim(5);

        await _counter.WaitAsync();

        TValue client;
        if (_connectionPool.TryPop(out client))
            return new ConnectionPoolWrapper<TValue>(client);

        return new ConnectionPoolWrapper<TValue>(initializer());
    }

    private ConnectionPoolWrapper(TValue value)
    {
        this._value = value;
    }

    /// <summary>
    /// The object that has been retrieved from the pool.
    /// </summary>
    private TValue _value;

    /// <summary>
    /// Specifies if this instance has been disposed. If it has been disposed then the value has been returned to the pool.
    /// </summary>
    /// <remarks>Type is <c>Int32</c> so that <c>Interlocked.Exchange</c> can be used.</remarks>
    private volatile int _disposed;

    /// <summary>
    /// Gets the object that has been retrieved from the pool.
    /// </summary>
    public TValue Value
    {
        get
        {
            var value = this._value;
            if (this._disposed != 0)
                throw new ObjectDisposedException(this.GetType().Name);

            return value;
        }
    }

    /// <summary>
    /// Returns the value in this wrapper to the pool.
    /// </summary>
    public void Dispose()
    {
        var disposed = System.Threading.Interlocked.Exchange(ref this._disposed, 1);

        // if it was previously disposed, do nothing
        if (disposed != 0)
            return;

        _connectionPool.Push(this._value);
        this._value = null;

        _counter.Release();

        GC.SuppressFinalize(this);
    }

    ~ConnectionPoolWrapper()
    {
        this.Dispose();
    }
}

I think the first change should be to make it possible to create multiple pools for the same type, probably maintaining a default static one based on type as it is currently.

What else should be changed to make this class actually ready for general usage without prior knowledge?

\$\endgroup\$
  • 1
    \$\begingroup\$ Given this is for .NET, I would make this connection pool behave the same as other connection pools in the framework. ConnectionType.Open() transparently creates a new connection or grabs one from the pool and ConnectionType.Close() or .Dispose() transparently returns the connection to the pool. Someone using the connection doesn't have to know about the connection pooling, just like you wouldn't for the myriad ADO.NET connection objects out there. \$\endgroup\$ – Dan Lyons Feb 18 '14 at 18:29
  • \$\begingroup\$ @DanLyons How would that work when you reach the maximum number of allowed connections? Would you make Open() async (similar to what the code in the question does)? \$\endgroup\$ – svick Feb 18 '14 at 18:45
  • \$\begingroup\$ As far as I can see, _connectionPool is static and one gets one pool per object type. Ideally one has one pool per server. Say 10 connection pool for one SMTP server and a 20 connection pool for another. \$\endgroup\$ – abuzittin gillifirca Feb 19 '14 at 12:47
4
\$\begingroup\$
  1. I think that syntax like ConnectionPool<SmtpClient>.RetrieveAsync() is more natural than your syntax. If you switch to that, you can probably eliminate the ConnectionPool/ConnectionPoolWrapper separation when it comes to static methods, which doesn't seem to serve any other purpose.

  2. Use usings to make your code shorter, there is no reason to write the full System.Threading.SemaphoreSlim every time you want to use that type.

  3. new System.Threading.SemaphoreSlim(5) the 5 here should be a named constant. Or it could be a property with default value, that can be modified. (Though in that case, I'm not sure what the property setter should do if it's invoked after the semaphore has already been created.)

  4. I think your initialization of _counter using double-checked locking is not actually thread-safe, the _counter variable would have to be volatile. For some more information, see Jon Skeet's article on implementing singletons.

  5. What's the point of the value local variable in the Value getter? If it's trying to be thread-safe, then I don't see a reason for that: the instance of ConnectionPoolWrapper should never be accessed from multiple threads at the same time.

  6. Some people recommend always using this. for accessing fields, some people recommend using a prefix like _. But I don't see any reason to use both at the same time.

\$\endgroup\$
  • \$\begingroup\$ Re: #5 - I made it thus because _disposed is volatile (making sure that CLR does not reorder operations) so if the instance gets accessed from multiple threads, the dispose check would still work. My goal was to make the wrapper itself thread-safe as well. \$\endgroup\$ – Knaģis Feb 18 '14 at 19:04
  • \$\begingroup\$ Re: #4 - wow, I did not expect this pattern of all things to be wrong... \$\endgroup\$ – Knaģis Feb 18 '14 at 19:10
  • \$\begingroup\$ @Knaģis In that case, I think you should either go fully-thread-safe or not try at all. As it is, the wrapper is not thread-safe, because calling Dispose() from two threads won't work correctly. \$\endgroup\$ – svick Feb 18 '14 at 19:14
  • \$\begingroup\$ @svik - yes, I did miss that little detail. I edited the code in question, it does seem to be thread safe now but I must admit, I'm not 100% sure. In the end there is still the possible issue if the caller just caches the result from .Value property... \$\endgroup\$ – Knaģis Feb 18 '14 at 19:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.