BuilderBase memory leak?

Apr 18, 2007 at 5:22 PM
Microsoft.Practices.ObjectBuilder.BuilderBase<TStageEnum> keeps a collection called lockObjects of type Dictionary<object, object>. This collection added to frequently, but nothing is ever removed. It looks like the key of the dictionary is always a Locator object (at least in the case I was looking at). Because of this, the instance was never GC and the collects the the locator held onto were also never GC. Because of this, I was getting objects that were never released and eventually the app would crash with a OutOfMemoryException. I have two questions:

1. Am I right about this being a memory leak?
2. Why is this collection used? I replaced:
lock (GetLock(locator))
{
// whatever
}
with

lock (locator)
{
// whatever
}

and everything seemed to work just fine.
Apr 18, 2007 at 7:41 PM
Edited Apr 18, 2007 at 10:13 PM
Because it would expose the internal lock model to external clients - violating locking model boundaries and, to an extent, object oriented principles.

For example, if I'm trying to build up an object, and I don't want a base class to violate my internal initialization sequence, I could take a lock out on the locator:
lock(_localLocator)
{
    // but then when I try to do the build up, if your suggestion were implemented,
    // the following would result in a deadlock, since BeginInvoke uses a ThreadPool
    // thread to call runBuildAsync
    EventHandler asyncBuildCall = new EventHandler(runBuildAsync);
    IAsyncResult result = asyncBuildCall.BeginInvoke(this, EventArgs.Empty, null, null);
    Thread.Sleep(0);                   // yield so the other thread starts
    asyncBuildCall.EndInvoke(result);  // block on call - thanks billsfan10
}
 
void runBuildAsync(object sender, EventArgs e)
{
    // Since we have a lock on _localLocator on another thread, 
    // this will hang in a deadlock
    _builder.Build(_localLocator, ...); 
}

Sure, this is a contrived example, but it could happen when building objects during, say, handling an EventSubscription on a background thread, or on the UI thread when it was raised on another thread.

Finally, the locator itself doesn't hold onto objects by their own handle, but via a DependencyResolutionKey, which uses a WeakReference internally to hold the object. The CLR ignores WeakReferences when constructing the reachable list. The locator instance isn't preventing collection of your objects - I'd check something else. I like to use WinDBG and sos.dll when tracking these references down.

Apr 18, 2007 at 9:47 PM
In your code above, the BeginInvoke() call would return right away and so the lock in that thread would be released. If you got the IAsyncResult from BeginInvoke() and called WaitOne() on it while in the lock, then you would deadlock.

The Locator has a dictonary of WeakReferences. The key of the dictionary is object. Those are strong references to DependencyResolutionKey instances. The lockObjects kept the Locator instances alive which kept the DependencyResolutionKey instances alive.

I used .NET Remote Performance Monitor that comes with .NET CF SP2 to see the number of instances of a given type. I could very easily see that DependencyResolutionKey instances were being created like crazy along with Locator. After my change, that didn't happen any more. I'm not seeing a problem with what I did.

NOTE: I know I didn't mention this before, but I'm talking about the Mobile Client Software Factory implementation.
Apr 18, 2007 at 10:06 PM
Edited Apr 19, 2007 at 5:26 PM
Good catch - I missed waiting on the IAsyncResult. I'll fix it so subsequent readers don't get confused. Even though this makes the problem always happen, not blocking makes it even harder to debug, since it could result in an intermittent deadlock - the thread could call BeginInvoke and be forced to yield only under certain circumstances. It shows why exposing locking syncs is never a good idea.

The DependencyResolutionKeys have two members: the key and the reference. They key is mostly a string. These are generally kept in the .Net runtime String intern pool - which is a per-process table (shared even across AppDomains). The GC doesn't collect these. You'll also occasionally see Type instances used as keys (such as in the ILifetimeContainer entry). Type instances aren't GC'd either. Holding references to either of these objects isn't a problem, since they stick around, references or not.

What are you using as a key? If it is a collectable object, try putting it in a WeakReference before using it as a key, if memory is your concern.