I'm a passionate web developer and systems architect. I work as an independent consultant specializing in EPiServer and search.
About web development with ASP.NET and EPiServer, fatherhood and other adventures in my digital jungle
I recently ran into a rather obscure but interesting problem on a project where we use EPiServer CMS, EPiAbstractions and Structure Map. At first, when the first request where made to the site after a release the site worked as expected but after having been up and running for a few minutes it started to feel less responsive and it gradually got worse. After about an hour it pretty much stopped responding. A quick look at the task managers performance tab showed the CPU usage at a 100%. In other words it behaved very much as if it had a memory leak, only it was a… CPU leak.
We profiled the application on a test server using dotTrace but at first we didn’t find anything that looked to be the cause of this problem. No method calls took an unreasonably long time to exectute or anything like that. However, after a while we noticed that there where a few methods that were called A LOT. Namely EPiAbstractions.DataFactoryFacade’s OnLoadingPage and OnLoadedPage were called 255 million times in just our profiling session which sampled me clicking around on the site for a few minutes.
As anyone who attended last weeks EPiServer user group meeting in Stockholm might recall from Stefan Forsberg’s presentation Structure Map uses a class’ most greedy constructor by default. The PageRepository class in EPiAbstraction, which we use extensively in our project, has two constructors:
public PageRepository(IDataFactoryFacade dataFactoryFacade,
IFilterForVisitorFacade filterForVisitorFacade)
{
dataFactory = dataFactoryFacade;
filterForVisitor = filterForVisitorFacade;
}
public PageRepository()
: this (DataFactoryFacade.Instance,
FilterForVisitorFacade.Instance) {}
The first constructor is primarily to enable testing and extendibility and requires it’s user to provide an implementation of, among other things, the IDataFactoryFacade interface. The other constructor has no parameters and passes the singleton implementations of EPiAbstractions default implementations on to the first constructor.
The DataFactoryFacade class is a complete wrapper for EPiServer’s DataFactory class that enables us to work against an interface that resembles DataFactory’s but that we can isolate our code from. As such it has all of the events that DataFactory has and in order to facilitate that it’s constructor will add some of the class’ own methods as event handlers to DataFactory’s events when executed. Below is a very shortened version of the class that illustrates this.
public class DataFactoryFacade : IDataFactoryFacade
{
public DataFactoryFacade()
{
DataFactory.Instance.LoadingPage += OnLoadingPage;
}
public event PageEventHandler LoadingPage;
public virtual void OnLoadingPage(Object sender, PageEventArgs e)
{
if (LoadingPage != null)
LoadingPage(sender, e);
}
}
So, with this in mind, what will happen each time we request an instance of the IPageRepository interface from our container if we have configured our container with the default conventions? It will create a new instance of the PageRepository class using it’s most greedy constructor. That will in turn require the container to create a new instance of the DataFactoryFacade class which will add event handlers to DataFactory’s events. I’m guessing that we requested an IPageRepository from the container about ten to thirty times per HTTP request in this site so for each HTTP request a lot of new handlers where added to DataFactory’s events. Since DataFactory is a singleton that is never disposed of if will keep those pointers to the event handlers for ever and execute them every time one of the events, such as loading a page, occurred.
The fix? There are several ways to resolve this but we took the easy way and told Structure Map to treat IPageRepository as a singleton, returning the same instance of it for each request for it.
If you’ve read all the way down here about this obscure problem I applaud you! On the other hand odds are you are one of two persons named Stefan and Christian ;-)
PS. For updates about new posts, sites I find useful and the occasional rant you can follow me on Twitter. You are also most welcome to subscribe to the RSS-feed.
This blog is built with EPiServer Community, EPiServer CMS, ASP.NET MVC and a bunch of other great products. The source code is available for download at the projects page, where you also can read more about this site and my other projects.
read more
Comments
Svante 1 years ago
Hi, nice catch!
However, as you state, you took the easy way out. This has nothing to do with StructureMap. You found a bug in your code, but you did not fix it - you just stopped triggering it in the particular case. You still have the basic issue - a public instance constructor adding an event to a singleton without removing it.
There's a great risk that you, or someone else, later will forget all about this and use the public instance constructor again - and thus retriggering the problem.
Bugs should be fixed - not avoided ;-) However, it's not really a trivial situation. But in the end you really need to ensure that this can't happen, either by ensuring that the reference to the event delegate is removed from the singleton, or by converting the DataFactoryFacade to a singleton as well - which actually makes some sense since it's a facade for a singleton...
It's an interesting design issue and a good example of why design matters.
I found an interesting article discussing the general issue of long-living event sources and shorter lived event listeners here: http://www.codeproject.com/KB/cs/WeakEvents.aspx .
Joel Abrahamsson 1 years ago
You are absolutely right Svante and I don't blame Structure Map in any way. It does however illustrate an interesting consequence of its convention to use the most greedy constructor.
Joel Abrahamsson 1 years ago
Also, I should probably also add that while EPiAbstractions is one of "my projects" I didn't see modifying it as within the scope of this specific customer project :)
Svante 1 years ago
Hmm... Since the issue really is with the public instance constructor adding an event listener to a singleton without a removal mechanism, are you sure you don't have this problem in other sites using this library, just a little less quickly catastrophic?
From what I've seen, the issue will always appear as soon as you create an instance of DataFactoryFacade that has a shorter lifetime than the DataFactory singleton instance, i.e. the AppDomain lifetime?
In other words - if you ever create a DataFactorFacade instance that you do not hold on to and re-use for the rest of the lifetime of the application you will actually have this issue - but perhaps it won't crash the app so quickly as the situation where you did find it.
Or did I miss something? I'm not familiar with EPiAbstractions and the patterns of use there, and the code posted is simplified, so perhaps my conclusions are incorrect, or at least irrelevant.
Joel Abrahamsson 1 years ago
Well, first of all you wont get any arguments from me regarding the flaw of adding the event handlers in a public constructor. That will either be fixed, give that I have the time, or fix itself as EPiServer introduces IDataFactory and thereby makes DataFactoryFacade obsolete.
However, you have to keep in mind that this post was not written to illustrate a feature of EPiAbstractions but a problem that I ran into as a user of it.
As for other sites experiencing this problem I think that most people that have used DataFactoryFacade has actually used its singleton implementation and therefore haven't had any issues. Neither would you normally have with PageRepository, unless of course you run into this issue.
As for the EPiAbstractions project in general I recently posted a series of posts about what it used to be (and for now still is), a wrapper around the untestable EPiServer API and what it tries to be these days along with some usage scenarios. I think that might serve as a good introduction to the project and I'd love som feedback on that.
Svante 1 years ago
Yes, I noticed that it was a singleton, and I guess the real lesson is to make sure a singleton is a singleton - i.e. never have a public constructor for a singleton - or don't make it a singleton.
I'm sure that you're right that most people have used the singleton implementation.
If, by chance, there is anyone who did not I hope this thread can be of assistance should they ever run into similar trouble.
It's a laudable project to provide a framework to facilitate TDD and unit testing of EPiServer code, which as you note is untestable without such a wrapper - I hope you continue to develop it. There is a great need of such!