Don’t dispose of objects that you don’t own

Don’t dispose of objects that you don’t own

  • Comments 10

In concurrent programs, race conditions are a fact of life but they aren’t all bad.  Sometimes, race conditions are benign, as is often the case with lazy initialization. 

The problem with racing to set a value, however, is that it can result in multiple objects being instantiated when only one is needed.  Take the LazyInitializer class that shipped in Visual Studio 2010 Beta 1, for example.  The method EnsureInitialized(ref T target, Func<T> valueFactory) accepts a ByRef value and a function that produces a value.  If target is null, EnsureInitialized will execute valueFactory and set target to the return value.  If multiple threads happen to overlap calls to EnsureInitialized, which is likely to be a rare occurrence, these threads may both initially see target as null and then execute valueFactory.  One thread will win the race and will get to set target

In Beta 1, we thought we’d be really smart by disposing of the objects that were created by threads that lost the race, if they implemented IDisposable.  Turns out that’s a pretty bad thing to do.  With lazy initialization, we assumed that most of time valueFactory would be creating a new value but that isn’t always the case.  It’s quite possible that a valueFactory could return an object that has already been created, in which case, we’d be disposing of an object we didn’t own. 

We’re making a couple of changes in the Parallel Extensions to ensure that we generally don’t dispose of object we aren’t sure we own and, moving forward, any new APIs that do eagerly instantiate objects that might not be used will not dispose of said objects. 

Now you might say, “why?  Ninety-percent of the time I am creating a new object with my value factory and it might be a really heavy object like a file or wait handle!”  We hear you, but typically, once an object is disposed, there is no bringing it back and so by disposing of these objects we don’t allow anyone to opt out of that behavior if need be.  If we don’t dispose, the GC will still cleanup your unused objects and if you really need to dispose of an object manually, there is a work around, i.e.

ManualResetEvent theEvent = null;
ManualResetEvent tmpEvent;
LazyInitializer.EnsureInitialized(ref theEvent, 
     () => { return tmpEvent = new ManualResetEvent(false); });
if (tmpEvent != theEvent) tmpEvent.Close();

In fact, you could easily create a small wrapper function to do the work for you, i.e.

public static T EnsureInitializedWithDispose<T>(ref T target, Func<T> function) 
    where T : class, IDisposable
{
    T tmp = default(T);
    T actual = LazyInitializer.EnsureInitialized(ref target, () => tmp = function());
    if (actual != tmp) ((IDisposable)tmp).Dispose();
    return actual;
}

And the issue isn’t just about race conditions.  In general, we should never dispose of an object we didn’t explicitly create.  BlockingCollection<T> in Beta 1, for example, will dispose of it’s underlying collection if the BlockingCollection<T> itself is disposed.  Again, this is bad news if you only were using the BlockingCollection<T> as a temporary wrapper.  In future releases of Visual Studio 2010, this will be corrected.

So when using these APIs, and any other API that may consume or create an IDisposable object, think about what that API is doing with the object and if you need to clean up resources manually.  If you’re not sure, the finalizer should take care of most of your problems.  In the rare situations where immediate cleanup is important, make sure you keep track of the objects and dispose of them properly.  Also, when designing your own libraries, don’t dispose of objects you don’t own!

Leave a Comment
  • Please add 8 and 7 and type the answer here:
  • Post
  • Shouldn't EnsureInitialized be overloaded to provide this disposal behaviour as an option?

    If you make this an option, it will yield greater benefit than just convenience. When people see the disposeUnusedObject parameter in IntelliSense, they'll be forced to think about the problem - which is surely a good thing!

    Otherwise, all but the most astute people will be unaware that the problem exists and will leave undisposed objects at the mercy of the GC's schedule. Which is OK with wait handles (which have a low OS burden), but not so cool with file handles (which can block access to the file!)

  • Nice posting!

    I think ownership of objects is something people spend much too little time thinking about (and documenting!) when it comes to garbage collected languages in general. In C++ for example, you need to be explicitly aware of who owns an object much more often, because someone is responsible for deallocating it at some point. Removing the need to explicitly free objects from memory also gives the illusion that all other questions of ownership (May I change this object I received from somewhere else, or do I have to make a copy? Can it change while I'm using it?  May/should I dispose of it when I'm done with it myself?) have been resolved automatically as well.

  • Hi Joe,

    Ideally, yes.  This is exactly what we had hoped to do.  Unfortunately, the harsh reality of shipping a product is that sometime you have to, well, ship.. And to do that, we have to stop churning code at some point.  Given that this API is really advanced and we don't expect many mainstream developers to use it (they'd use Lazy<T> instead) we decided not to prioritize adding an overload.  Those that will use it, are likely to be very concerned with things like file handles and make sure they're disposed properly.

    Thanks!

    Josh

  • I would concur with Kristian's point, a lot of developers seems to mentally broaden the GC's remit from 'memory' to 'general responsibility' collector. Gosh how I wish I could declare a const reference to a object in C# ...

  • Shouldn't the call to compare be to ReferenceEquals on Object?  If for some reason, compare-by-value semantics has been implemented in the equality operator (which best practice indicates you should implement != as well), then there is an issue of disposing the wrong object here.

  • Hi Nicholas,

    Good question. In most cases, that would be true, but in this case, the restrictions on generics actually helps(?) out here.  

    In C# overloading operators on generic types is not supported and thus inside of the generic method, != will not resolve to the overriden implementation.  You do have a good argument for readability, however!

    Josh

  • Hi,

    Indeed, object ownership is an issue with the disposable pattern, which is why the right thing to do is indeed to let the user deal with object disposal (or provide an explicit overload).

    Also, I was looking at the beta 1 code for LazyInitializer and noticed that you use an acquire barrier on the public methods, but I can't seem to figure out what it's for, with my understanding of the memory model (is there going to be a formal memory model specification, based on Prism?).

    Thanks!

    Duarte

  • Hi Duarte,

    We read a dummy volatile variable to prevent potential memory reordering in IA64.  

    Thanks!

    Josh

  • Hi Josh,

    Thanks for the answer! I guess you mean the potential load-load reordering. Does this mean the memory model for 4.0 that we developers must address is the weakest (i.e., IA64's)?

    Thanks again,

    Duarte

  • That, of course, depends on the architecture(s) you are targeting. :)  

    Josh

Page 1 of 1 (10 items)