2

I have Client which uses a disposable Connection for talking to a remote service. A Connection is somewhat expensive to set up and needs to be Dispose()d properly.

I want to allow multiple methods in Client to share a connection, like this:

using (client._connection = client.Connect())
{
  client.SyncTime();
  client.UpdateUsers();
  client.SomeOtherOperation();
}

But I don't want to insist on Client's consumers managing connections. They should be allowed to call individual single Client methods without being aware or concerned with Connections.

Here is the pattern I'm using at the moment. It makes SRP cry. Any suggestions on how I can do this better, or should I be doing this at all?

public class Client
{
  Connection _connection;

  public void SyncTime()
  {
    bool wasConnected = _connection == null;
    try
    {
      if (!wasConnected)
        _connection = Connect();

      _connection.ActuallyDoSomeWork();
      ...

    }
    finally
    {
      if (!wasConnected)
        _connection.Dispose()
    }
  }
}
1
  • You start by saying you want to be able to call multiple methods on Client using the same connection, but the code you're using right now suggests each of those methods will create and dispose a connection each time they're called. You can't have it both ways - if you want to batch operations, the users of Client need to be aware that their tasks will be batched even if you never allow them to touch the connection object directly.
    – Doval
    Jul 18, 2014 at 18:29

2 Answers 2

2

The first thing you need to do is explicitly separate out the connection lifecycles in your two scenarios. Trying to juggle the two will only lead to confusion. To that end, I would completely remove the _connection member variable.

Here's what I propose:

Use a "context object" to manage your state. Your Connect method already returns a Connection, so that can serve as your context object for now. Make all your functional API methods SyncTime, UpdateUsers, etc. always take in this context object and act accordingly.

So your implementation becomes:

public class Client
{    
    public void SyncTime(Connection connection)
    {
        if (connection == ONE_TIME_CONNECTION)
        {
            using (Connection localConnection = client.Connect())
            {
                SyncTimeInternal(localConnection); 
            }
        }
        else
        {
            SyncTimeInternal(connection);
        }
    }

    private void SyncTimeInternal(Connection connection)
    {
        DoSomeWork();
        connection.DoSomeMoreWork();
        EvenMoreWork();
    }

    public static final Connection ONE_TIME_CONNECTION = null;
}

Then your consumer code becomes:

using (Connection connection = client.Connect())
{
  client.SyncTime(connection);
  client.UpdateUsers(connection);
  client.SomeOtherOperation(connection);
}

or just:

client.SyncTime(Client.ONE_TIME_CONNECTION);

Notice the use of the constant to abstract away a null value. This makes it easier to change the internals in the future.

Of course, you can extend this pattern to create an explicit ClientContext object containing the connection as well as other state information.

1
  • The "constant == null" looks interesting. If nothing else, it allows you to use null parameters in a self-documenting way. I don't suppose you can point to a larger example/discussion of this? Nov 25, 2014 at 22:17
1

You could wrap your disposable Connection in a reference-counted disposable wrapper, like so:

public abstract class RefCountDisposable<TDisposable> : IDisposable where TDisposable : class, IDisposable
{
    TDisposable reference;

    protected RefCountDisposable(TDisposable reference)
    {
        this.reference = reference;
    }

    public TDisposable Reference { get { return reference; } private set { reference = value; } }

    public bool IsDisposed { get { return reference == null; } }

    public void Dispose()
    {
        // Dispose of unmanaged resources.
        Dispose(true);
        // Suppress finalization.  Since this class actually has no finalizer, this does nothing.
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (IsDisposed)
            return;
        if (disposing)
        {
            // Free any other managed objects here. 
            Reference = null;
        }
        // Free any unmanaged objects here. 
    }

    public override string ToString()
    {
        if (IsDisposed)
            return base.ToString() + ": Disposed";
        else
            return base.ToString() + ": " + reference.ToString();
    }
}

public sealed class RefCountDisposableFactory<TDisposable> where TDisposable : class, IDisposable
{
    Func<TDisposable> getReference;
    TDisposable reference;
    int refCount;
    object padlock = new object();

    sealed class RefCountDisposable : RefCountDisposable<TDisposable>
    {
        readonly RefCountDisposableFactory<TDisposable> factory;

        internal RefCountDisposable(RefCountDisposableFactory<TDisposable> factory, TDisposable reference)
            : base(reference)
        {
            if (factory == null)
                throw new ArgumentNullException("factory");
            this.factory = factory;
        }

        protected override void Dispose(bool disposing)
        {
            if (IsDisposed)
                return;
            try
            {
                if (disposing)
                {
                    // Free any other managed objects here. 
                    factory.DisposeReference(Reference);
                }
                // Free any unmanaged objects here. 
            }
            finally
            {
                base.Dispose(disposing);
            }
        }
    }

    public RefCountDisposableFactory(Func<TDisposable> getReference)
    {
        this.getReference = getReference;
        this.reference = null;
        this.refCount = 0;
    }

    public RefCountDisposable<TDisposable> Create()
    {
        lock (padlock)
        {
            if (reference == null)
                reference = getReference();
            refCount++;
        }

        return new RefCountDisposable(this, reference);
    }

    void DisposeReference(TDisposable reference)
    {
        if (reference == null)
            return; // already disposed.
        lock (padlock)
        {
            if (reference != this.reference)
            {
                string msg = string.Format("invalid reference {0}", reference);
                Debug.Assert(false, msg);
                throw new ArgumentException(msg);
            }
            if (refCount <= 1)
            {
                if (reference != null)
                    reference.Dispose();
                reference = null;
                refCount = 0;
            }
            else
            {
                refCount--;
            }
        }
    }
}

The idea here is that the RefCountDisposableFactory allocates a Connection the first time you need it and returns it in a RefCountDisposable<Connection> wrapper. Subsequent requests for a Connection increment a reference count and return new wrappers to the same reference Connection. Once the last wrapper is disposed, the Connection is disposed and set to null so that subsequent requests will create a new instance.

My prototype factory isn't robust against somebody disposing the underlying disposable reference directly. Do you think it should be?

I also note Microsoft itself has something called RefCountDisposable, but it isn't generic and I can't find any useful documentation.

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.