I posted earlier that I'm switching to blogengine.net, as part of this I've been fiddling around with the code (as is my way..I'll contribute back to the source when I've finished). One of my major pet hates is poor use of the singleton pattern, especially as there's a definitive article on the pattern in .NET and how to do it well. It's actually likely that this pattern is overkill in this case and a ReaderWriterLockSlim could be better (though it has it's own problems) . Anyway, on the assumption that a Singleton is the best choice here, let's look at the current code:
public static BlogSettings Instance
{
get
{
if (blogSettingsSingleton == null)
{
blogSettingsSingleton = new BlogSettings();
}
return blogSettingsSingleton;
}
}
If you look at the article I mentioned above you'll see that this is the version which is specifically called out as follows:
"the above is not thread-safe. Two different threads could both have evaluated the test if (instance==null) and found it to be true, then both create instances, which violates the singleton pattern. Note that in fact the instance may already have been created before the expression is evaluated, but the memory model doesn't guarantee that the new value of instance will be seen by other threads unless suitable memory barriers have been passed.'
The common 'best' singleton pattern (well, it's debatable...but generally the best...) is a lot more wordy (see version 5 in that article) so I was please to find this post on a Generic Singleton (actually this was posted a while ago on Codeproject)...really nice. In the Utils class I added this:
/// <summary>
/// Provides a Singleton implementation using Generics.
/// </summary>
/// <typeparam name="T">Type of singleton instance</typeparam>
public sealed class Singleton<T> where T : new()
{
Singleton() { }
public static T Instance
{
get
{
return Nested.instance;
}
}
class Nested
{
// Explicit static constructor to tell C# compiler
// not to mark type as beforefieldinit
static Nested() { }
internal static readonly T instance = new T();
}
}
The code for returning the instance then becomes:
public static BlogSettings Instance
{
get
{
return Utils.Singleton<BlogSettings>.Instance;
}
}
You have to also change the constructor for BlogSettings to public to allow this this to work which does let devs shoot themselves in the foot (by ignoring the singleton) and you of course have to balance the benefit agains that risk...