Implementing IDisposable correctly to return TCP connection back to pool

Question

I'm trying to implement TCP connection pooling and return a connection back to the pool using IDisposable. I'm wondering if my implementation is correct. It seems to be working but I think because the base class also implements IDisposable and finalize, my code might be leaky.

public class BaseClass : IDisposable
{
    internal bool IsDisposed { get; set; }
    private object someResource;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~BaseClass()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (someResource != null)
        {
            // some clearn up
            return;
        }

        if (disposing)
        {
            //dispose un managed resources

        }
    }
}

public class ChildClass : BaseClass
{
    // adds some functionality
}

public class MyClass : ChildClass, IDisposable
{
    MyPoolManager manager = null;

    public MyClass(MyPoolManager manager)
    {
        this.manager = manager;

    }
    void IDisposable.Dispose()
    {
        manager.ReturnPooledConnection(this);
    }
}

public class MyPoolManager
{
    private static MyPoolManager instance = new MyPoolManager();
    private static object objLock = new object();

    private static Queue<MyClass> que = null;
    private string name;

    static MyPoolManager()
    {
        que = new Queue<MyClass>();
        // enqueue some instances of MyClass here
        MyClass client = new MyClass(instance);
        que.Enqueue(client);
    }

    private MyPoolManager() { }

    public MyPoolManager(string name)
    {
        this.name = name;
    }


    public MyClass GetPooledConnection()
    {
        lock (objLock)
        {
            while (que.Count == 0)
            {
                if (!Monitor.Wait(objLock, 1000))
                    throw new TimeoutException("Connection timeout");
            }
            return que.Dequeue();
        }
    }

    public void ReturnPooledConnection(MyClass client)
    {
        lock (objLock)
        {
            que.Enqueue(client);
            Monitor.Pulse(objLock);
        }
    }
}

Use it like this in your program:

MyPoolManager pool = new MyPoolManager();
using (var conn = pool.GetPooledConnection())
{
    // use the conn here
}

// when you reach here the conn should have returned back to the pool

Answer 1

The days of using IDisposable for things like this only came about because of the lack of functional programming concepts in C#. Now that Action and Func<T> exist there's absolutely no reason to use IDisposable for this situation.

A pool of connections/resources can expose a much nicer and simpler interface that removes the need for the using statement altogether and as a result avoids the possibility of the caller "forgetting" to give the resource back to the pool. Just because something implements IDisposable it doesn't mean that the caller will dispose of it properly.

A few points on your implementation:

• Your BaseClass is completely unnecessary and can be removed.
• The manager class has a poor implementation of what's already in the .NET framework: ConcurrentQueue. It's not only a better option because you don't have to write it, but it's lock-free and much more efficient.
• The manager class's lock forces serialised access to one thread at a time, even if there are plenty of resources available. If you have 30 connections, you should let 30 threads access your resource pool at once, not just 1. Use a Sempahore instead.

So with this in mind I'd like to make a couple of suggestions.

1. Generalise your resource pool. The resource pool shouldn't be coded to a specific type of thing. Anything of any type can be pooled.
2. Avoid the use of disposable and use higher order functions to wrap up the lifetime of the resource. The consumer can not possible forget to give the resource back to the pool because you're doing it all for them.
3. Use proper .NET collections to solve the problem, not your own implementation.
4. Avoid a straight lock which serialises access when you can instead use a semaphore.
5. Don't push the logic of resource creation into the pool. Pass in a function that can do the creation for you, then you can have something that can create objects of arbitrary complexity.

Here's a quick implementation I've just thrown together (haven't compiled it, it's for reference and for ideas only):

public class Pool<TResource>
{
  private const int PoolWaitTimeoutMs = 1000;

  private readonly BlockingCollection<TResource> _resources;

  public Pool(int size, Func<TResource> producer)
  {
    _resources = new BlockingCollection<TResource>();

    for(var i = 0; i < size; ++i)
    {
      _resources.Add(producer());
    }
  }

  public void Consume(Action<TResource> consumer)
  {
    // This is a helper for when consumers don't produce a result
    Consume(new Func<TResource, object>(res => { consumer(res); return null; }));
  }

  public TResult Consume(Func<TResource, TResult> consumer)
  {

    TResource res;

    try
    {
      // wait for a resource to become available
      if (!_resource.TryTake(out res, PoolWaitTimeoutMs))
      {
        throw new Exception("Unable to acquire resource");
      }

      // invoke the consumer function and return the result
      return consumer(res);
    }
    finally
    {
      // put it back in the queue if we managed to get one
      if(res != null)
      {
        _resources.Add(res);
      }
    }
  }
}

Using the pool them becomes as simple as this:

public class SomePooledResource
{
  public void DoStuff() { ... };
  public List<string> GetStuff() { ... };
  // other stuff
}

// create a pool of 10 resources
var pool = new Pool<SomePooledResource>(10, () => new SomePooledResource);

// use the resource without returning something
pool.Consume(res => res.DoStuff());

// use the resource returning a result
var results1 = pool.Consume(res => res.GetStuff());

// use the resource to do both
var results2 = pool.Consume(res =>
    {
      res.DoStuff();
      return res.GetStuff();
    });

Answer 2

I have basically introduced a wrapper for connection and also IDisposable on PoolManager, so it will dispose the connections when it is disposed. And also edited the Dispose methods slightly.

 public class BaseClass : IDisposable
 {
    internal bool IsDisposed { get; private set; }
    private object someResource;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~BaseClass()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (IsDisposed) return;

        if (disposing)
        {
           if (someResource != null)
           {
               // some clean up
           }

           //dispose unmanaged resources    
        }

        someResource = null;
        IsDisposed = true;
    }
}

public class ChildClass : BaseClass
{
    // adds some functionality
}

public class PoolWrapper : IDisposable
{
    private MyPoolManager manager;
    public BaseClass Connection { get; private set; };

    public PoolWrapper(MyPoolManager manager, BaseClass connection)
    {
        this.manager = manager;
        this.Connection = connection;
    }

    void IDisposable.Dispose()
    {
        manager.ReturnPooledConnection(this);
    }
}

public class MyPoolManager : IDisposable
{
    private static MyPoolManager instance = new MyPoolManager();
    private static object objLock = new object();

    private static Queue<BaseClass> que = null;
    private string name;
    internal bool IsDisposed { get; private set; }

    static MyPoolManager()
    {
        que = new Queue<BaseClass>();
        // enqueue some instances of MyClass here
        que.Enqueue(client);
    }

    private MyPoolManager() { }

    public MyPoolManager(string name)
    {
        this.name = name;
    }

    public PoolWrapper GetPooledConnection()
    {
        lock (objLock)
        {
            while (que.Count == 0)
            {
                if (!Monitor.Wait(objLock, 1000))
                    throw new TimeoutException("Connection timeout");
            }
            return new PoolWrapper(this, que.Dequeue());
        }
    }

    public void ReturnPooledConnection(PoolWrapper client)
    {
        lock (objLock)
        {
            que.Enqueue(client.Connection);
            Monitor.Pulse(objLock);
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~MyPoolManager()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!IsDisposed && disposing)
        {
            while (que.Count != 0) 
                  que.Dequeue().Dispose();   
        }

        IsDisposed = true
    }
}

And you can use it like this in your program:

 using (var pool = new MyPoolManager())
 using (var instance = pool.GetPooledConnection())
 {
      instance.Connection.DoSomething();
 }