Chill out on the Singleton Fetish

My little team is taking over development for one of our other products.  Today is the first day of the typical “I’m getting the app up on my box and the NAnt build doesn’t work on my box” hell.  One of the things I’m seeing in trying to debug the broken unit tests is a bag of Singleton’s that perform data access.  Just to make it more fun, one singleton uses another singleton to get its configuration.  My real problem is that the database connection string isn’t correct yet, but the test failures exposed some larger structural issues to address later. 


Look at the code below for a second.  It’s nothing special really, just a generic Singleton that serves as a gateway to some kind of resource, but this little bit of innocuous code can thoroughly hose the testability of any application.


 

	public class Singleton
{
private static Singleton _instance;

// Lazy initialization because that’s more *efficient*
public static Singleton GetInstance()
{
if (_instance == null)
{
lock (typeof(Singleton))
{
if (_instance == null)
{
_instance = new Singleton();
}
}
}

return _instance;
}

private object _something;

private Singleton()
{
_something = getSomethingFromDatabase();
}

private object getSomethingFromDatabase()
{
// go fetch some kind of configuration information from the database
return null;
}
}


So what’s so wrong with this?  Plenty.  The instance of Singleton touches the database in its constructor function.  Unit testing any class that depends on the Singleton class automatically includes some sort of live data access – or does it?  One of the problems with singleton’s in unit testing is the lack of control over when the singleton was created.  To the best of my knowledge, NUnit does not guarantee the order of unit test execution, so the singleton instance will be instantiated by whichever unit test happens to run first.  Why is this such a bad thing?  Because your tests aren’t starting from a known state and therefore the results of the test are not reliable.


For another thing your unit test isn’t a unit test, it’s an integration test. Integration tests are cool too, but unit tests might be more important from the standpoint of creating working code.  One of the qualifications and benefits of a unit test is that it pinpoints the exact trouble spot in the code when it fails.  If a coarse-grained test fails, you’ve got to look in a lot more code to find the exact problem.  If there’s a database involved, then you’re troubleshooting search widens outside of your code.  It’s possible to test against a live database if you can control the database state, but why do all that extra work if you don’t need to?  Solve one problem at a time and data access is often a separate issue.  One obvious way to tell whether code is adequately unit tested at a fine-grained level is the amount of time spent with the debugger to fix problems.  If you’re using the debugger a lot, you really need to reconsider your unit testing approach. 


Forget TDD for a second.  Using a stateful singleton opens yourself up to all kinds of threading issues.  Not many developers that write business software are going to be threading experts.  When you screw up threading safety you can create really wacky bugs that are devilishly hard to reproduce.  You do love bugs that can’t be reproduced don’t you?  The code I’m looking at today seems to handle the thread safety issues quite well, but all the “lock(this, that, or the other)” code blows up the lines of code count.  It adds complexity to the code and makes the code harder to understand and read.  Add in a bunch of tracing code and you end up with a whole lot of noise code surrounding a little piece of code that actually does something useful.  My biggest issue with the code is whether or not it was really necessary in the first place.  One of the truisms in software development you bump into occasionally is that “Premature Optimization is the root of all evil” in software development.  I’m guessing that the singletons were put in place due to performance concerns.  Do it a simple way first and forget caching out of the gate.  The performance bottlenecks are almost never where you think they’ll be anyway.  By doing things the simple way upfront you can leave yourself with more time later when the real performance problems become evident.  Don’t do anything that makes the code harder to understand unless you absolutely have to.


My best advice is to not use caching via static members until you absolutely have to for performance or resource limitation reasons.  If you do need singleton-like functionality, you might try some of the techniques from this article.  You can also get around the singleton issues by just being able to replace the single instance with a mock or stub from a static member.  It works, but it’s not my preference.


 

About Jeremy Miller

Jeremy is the Chief Software Architect at Dovetail Software, the coolest ISV in Austin. Jeremy began his IT career writing "Shadow IT" applications to automate his engineering documentation, then wandered into software development because it looked like more fun. Jeremy is the author of the open source StructureMap tool for Dependency Injection with .Net, StoryTeller for supercharged acceptance testing in .Net, and one of the principal developers behind FubuMVC. Jeremy's thoughts on all things software can be found at The Shade Tree Developer at http://codebetter.com/jeremymiller.
This entry was posted in Test Driven Development. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Err

    @Brian – This was mentioned by Rob in the comments above yours. Valid point though.

  • http://brianc.me/blog Brian

    err…lock(typeof(Singleton))…

  • http://brianc.me/blog Brian

    this might seem silly, and I’m not trying to nitpick, simply sharing: Something I learned recently…Best not to lock on a type object as you can potentially run into a deadlock if another thread locks on the same type object and then requests the singleton for the first time.

    public void ICodeLikeAJerkWad()
    {
    lock(Singleton)
    {
    var deadLockPwnation = Singleton.GetInstance()
    }
    }

    In general, when using the lock keyword, best bet is to lock on a private scoped instance of something, generally something like:

    private static Object sync = new Object();
    lock(sync)
    {
    //try to understand concurrency here
    }

  • http://codebetter.com/blogs/jeremy.miller jmiller

    Thanks Rob, the original code did lock on an object instance in their singleton, I introduced the lock on the type in my sample. I get your point though.

  • http://devauthority.com/blogs/devprime/default.aspx Rob

    Especially on classes that work in a service environment, locking is a necessary evil, but one that shouldn’t be done carelessly. Locking isn’t just a problem for singleton classes, as even instance classes can be accessed by multiple threads. Having said that, not locking in the *right* places is bad, and locking too much can be equally as bad. However, one problem that should be addressed here is try *not* to lock on types. That’s generally a Bad Thing ™, even though a lot of older MSDN samples showed it being done. It’s best to use a specfic object instance as a lock, not a type.

  • Gary Williams

    Actually, the singleton in question was used to wrap the static M/S SQL*Helper class. It was a very early part of the implementation on a team that was just beginning to learn TDD.

    Was it a mistake? Possibly. The architecture was completely refactored part way through the project and the original rationale for the singleton was largely lost. It’s an item that is a candidate for a refactor, certainly…but there are always lots of those.

  • http://codebetter.com/blogs/jeremy.miller jmiller

    Thanks Scott.

    If I do a singleton these days I just go for the “private static instance = new Singleton();” approach and do it upfront.

  • http://www.OdeToCode.com/blogs/scott/ Scott Allen

    Actually – that kind of double check locking in GetInstance can have threading issues :) The first check of _instance == null is looking at a “work in progress” in shared memory without taking a lock. It might see _instance become non-null before all the ctor writes are finished. One recommended solution is to use the volatile keyword with the _instance field declaration ….

    … but I agree, this doesn’t sound like the best place for a singleton.