One of our apps was leaking memory. This passed unnoticed for few years but as the app is used by more and more users, the leak became problematic. The app pool memory had been growing to few gigabytes until the pool was recycled every 24 hours.
The first thing to do in such case is to determine if the leak is caused by a managed code or rather an unmanaged code. This can be done with performance counters. Then, the DebugDiag can be used to collect a memory dump and then the dump can be analyzed to find what objects occupy the memory.
In our case it turned out the Unity container was the culprit or rather, an unfortunate implementation of a unity controller factory for ASP.NET MVC:
public class UnityControllerFactory :
IControllerFactory
{ #region IControllerFactory private IUnityContainer _container; private IControllerFactory _innerFactory; public UnityControllerFactory( IUnityContainer container ) : this( container, new DefaultControllerFactory() )
{ }
protected UnityControllerFactory( IUnityContainer container, IControllerFactory innerFactory )
{ _container = container;
_innerFactory = innerFactory;
}
public IController CreateController( RequestContext requestContext, string controllerName ) { if ( _container.IsRegistered<IController>( controllerName.ToLowerInvariant() ) ) return _container.Resolve<IController>( controllerName.ToLowerInvariant() ); var controller = _innerFactory.CreateController( requestContext, controllerName );
if ( controller is IBuildUpController )
{ var controllerContainer = _container.CreateChildContainer() )
( (IBuildUpController)controller ).PreBuildUp( controllerContainer );
controllerContainer.BuildUp( controller.GetType(), controller );
( (IBuildUpController)controller ).PostBuildUp( controllerContainer );
}
return controller; }
public void ReleaseController( IController controller )
{ if ( controller is IDisposable )
( (IDisposable)controller ).Dispose();
_container.Teardown( controller );
}
public System.Web.SessionState.SessionStateBehavior GetControllerSessionBehavior( RequestContext requestContext,
string controllerName ) { return System.Web.SessionState.SessionStateBehavior.Default; }
#endregion }
There is an idea behind such factory – it tries to be smart.
If a controller type is registered, it uses the container to resolve an instance. This is to allow constructor injection.
If a controller type is not registered, it still allows property injection assuming the type is decorated with an attribute.
Since this is a multitenant app, the factory uses a child container to be able to configure services differently for different tenants (obviously, looking at the code, the idea of conditional registrations depending on the tenant was never implemented).
Unfortunately also, this is not obvious why this code lacks the memory.
The problem is the way Unity creates a child container. By inspecting the source code I can learn that the CreateChildContainer method actually calls new UnityContainer( this ) which is implemented as
private UnityContainer(UnityContainer parent) { this.parent = parent; if (parent != null)
{ parent.lifetimeContainer.Add(this); // <- culprit!!!
}
...
}
The obvious thing here is that the child registers itself in the lifetime container of the parent which means that consecutive children will make the list grow and grow. And of course there is a way to “unregister”:
protected virtual void Dispose(bool disposing)
{ if (disposing) { if (lifetimeContainer != null)
{ lifetimeContainer.Dispose();
lifetimeContainer = null; if (parent != null && parent.lifetimeContainer != null)
{ parent.lifetimeContainer.Remove(this); // <- ha!
}
}
extensions.OfType<IDisposable>().ForEach(ex => ex.Dispose());
extensions.Clear();
}
}
which in turn leads to an obvious fix to the original unity factory
public IController CreateController( RequestContext requestContext, string controllerName ) { if ( _container.IsRegistered<IController>( controllerName.ToLowerInvariant() ) ) return _container.Resolve<IController>( controllerName.ToLowerInvariant() ); var controller = _innerFactory.CreateController( requestContext, controllerName );
if ( controller is IBuildUpController )
{ using ( var controllerContainer = _container.CreateChildContainer() ) { ( (IBuildUpController)controller ).PreBuildUp( controllerContainer );
controllerContainer.BuildUp( controller.GetType(), controller );
( (IBuildUpController)controller ).PostBuildUp( controllerContainer );
} // <- this calls Dispose and unregisters the child from the parent }
return controller; }
An important issue that potentially occurs here is what happens in a concurrent environment where controllers are created upon user requests so that child containers are constantly added and removed from the parent container list. And of course, List<T> methods for adding/removing items are not thread safe.
It turns out fortunately, that Unity doesn’t use just a List<T> for the lifetimeContainer list, rather it has a custom implementation of a synchronized list that locks adding and removing making it thread safe.
Lesson learned, leak fixed, warning dismissed.
No comments:
Post a Comment