views:

301

answers:

6

I have an object that is expensive to create, which uses some unmanaged resources that must be explicitly freed when done with and so implement IDisposable(). I would like a cache for instance of these expensive resources so that the creation cost is minimized, but I am having trouble knowing how to deal with the disposal.

If the methods that use the objects are responsible for the disposal then I end up with disposed instances in the cache, which then have to be recreated, defeating the point of the cache. If I don't dispose the objects in the methods that use them then they never get disposed. I thought I could dispose them when they are taken out of the cache, but then I might end up disposing an instance which is still being used by a method.

Is it valid to just let them go out of scope and be collected by the garbage collector and free up the resources at that point? This feels wrong and against the idea of them being disposable...

+2  A: 

You could decouple the unmanaged resources from the managed instance and use a cache manager to hold a set of unmanaged resources. The managed object will try to acquire an instance of the unmanaged resource from the cache manager which will either create one or give one free instance from the cache and return it to the cache manager (instead of disposing it itself) at the time of its disposal. The cache manager will be the sole responsible object for allocating and freeing unmanaged resources when it sees it's necessary.

Mehrdad Afshari
+3  A: 

First of all, the type that wraps the native resources should be finalizable, not just disposable. Even better, use SafeHandle to wrap the native resources.

Unless someone is explicitly responsible for saying they are done with the item and it can be disposed, then I think you're better off letting the GC take care of it. Note that it must be finalizable though, otherwise the GC won't give it a second glance.

HTH, Kent

Kent Boogaart
+3  A: 

To (mis-)quote Raymond Chen: Every cache without an expiration policy is a leak

So, set a clear cache expiration policy, and let the cache dispose them as the normal case. This still leaves application shutdown to be handled.

If your unmanaged ressources are owned by the process, you can let the process release them on shutdown.

If the unmanaged ressources are not owned by the process, you need to detect shutdown and explicitely Dispose the cached elements.

If you can't detect process shutdown reliably, and the managed ressources are expensive, the unmanaged ones are not, separate the managed from the unmanaged ressources, and let the cache keep only the managed ones.

When the unmanaged ressources are expensive so they need caching, and they are not owned by the process, and you cannot detect process shutdown reliably and you cannot afford to leak them, then your problem cannot be solved.

peterchen
A: 

An object should be disposed by the class that creates it. Since your callers have not created the items in the cache, they have no business disposing of them, either.

I'd want to make sure that your factory method is named something like "Get*Class*" rather than "Create*Class*" in order to emphasize that the caller is not responsible for creation, and therefore not for disposal.

John Saunders
"creator is owner" is a possible design guideline, but as your answer already indicates: it is not always clear who is the creator (the factory or the factory client?). It's preferable to clearly and explicitly document this. As my own answer shows, you can even transfer ownership then.
Wim Coenen
+1  A: 

You can solve this with a class factory and IDisposable. For example:

public class CachedObject : IDisposable {
  private int mRefCount;
  private CachedObject(int something) {
    mRefCount = 1;
  }
  public static CachedObject CreateObject(int something) {
    CachedObject obj = LookupInCache(something);
    if (obj != null) Interlocked.Increment(ref obj.mRefCount);
    else obj = new CachedObject(something);
    return obj;
  }
  private static CachedObject LookupInCache(int something) {
    CachedObject obj = null;
    // Implement cache lookup here, don't forget to lock
    //..
    return obj;
  }
  public void Dispose() {
    int cnt = Interlocked.Decrement(ref mRefCount);
    if (cnt == 0) {
      // Remove from cache
      // 
    }
  }
}
Hans Passant
This solution can have some unexpected effects because it skews the concept of ownership. The client code is responsible for calling dispose, but if the object is mutable it also needs to be aware that there may be other owners out there working with the same object.
Wim Coenen
No, if it is mutable then it shouldn't be cached.
Hans Passant
This class isn't threadsafe - your interlocked inc/dec's don't protect the if guards.
Eamon Nerbonne
(basically, just use locks instead...)
Eamon Nerbonne
+2  A: 

Disposable objects always need to have a clear owner who is responsible for disposing them. However, this is not always the object that created them. Furthermore, ownership can be transferred.

Realizing this, the solution becomes obvious. Don't dispose, recycle! You need not only a way to acquire a resource from the cache, but also a way to return it. At that point the cache is owner again, and can choose to retain the resource for future use or to dispose it.

   public interface IDisposableItemCache<T> : IDisposable
      where T:IDisposable 
   {
      /// <summary>
      /// Creates a new item, or fetches an available item from the cache.
      /// </summary>
      /// <remarks>
      /// Ownership of the item is transfered from the cache to the client.
      /// The client is responsible for either disposing it at some point,
      /// or transferring ownership back to the cache with
      /// <see cref="Recycle"/>.
      /// </remarks>
      T AcquireItem();

      /// <summary>
      /// Transfers ownership of the item back to the cache.
      /// </summary>
      void Recycle(T item);

   }
Wim Coenen
I basically ended up with a solution that was a hybrid of this and NoBugz answer. Thanks
Sam Holder