Death of a Singleton

We’re now up to the seventh instalment of Extreme ASP.NET Makeover. In part 6, we moved away from the UI a little and worked more in the code behind the pages. In this article, we’ll continue that work and focus on a specific pattern that is used through the codebase: the Singleton.

Let’s review one of the changes we made to the code in part 6. In that discussion, we refactored the authorization checking into a separate class, AuthorizationServices. To do that, we moved a lot of code from the Page_Load method of our Default.aspx page into it. Figure 1 shows an excerpt from the RetrieveCurrentStatusFor method.

Figure 1 Excerpt from the RetrieveCurrentStatusFor Method

return new CurrentCapabilitiesStatus
{
    CanView = AuthChecker.Instance.CheckActionForPage(
        currentPage, Actions.ForPages.ReadPage,
        currentUsername, currentGroups),
    CanDownloadAttachments = AuthChecker.Instance.CheckActionForPage(
        currentPage, Actions.ForPages.DownloadAttachments,
        currentUsername, currentGroups),
    CanSetPerms = AuthChecker.Instance.CheckActionForGlobals(
        Actions.ForGlobals.ManagePermissions,
        currentUsername, currentGroups),
    CanAdmin = AuthChecker.Instance.CheckActionForPage(
        currentPage, Actions.ForPages.ManagePage,
        currentUsername, currentGroups),
    CanViewDiscussion = AuthChecker.Instance.CheckActionForPage(
        currentPage, Actions.ForPages.ReadDiscussion,
        currentUsername, currentGroups),
    CanPostDiscussion = AuthChecker.Instance.CheckActionForPage(
        currentPage, Actions.ForPages.PostDiscussion,
        currentUsername, currentGroups),
    CanManageDiscussion = AuthChecker.Instance.CheckActionForPage(
        currentPage, Actions.ForPages.ManageDiscussion,
        currentUsername, currentGroups),
    CanEdit = canEdit,
    CanEditWithApproval = canEditWithApproval
};

Notice all the calls to AuthChecker.Instance. This is an example of the Singleton Design Pattern at work. We’ll define it now to get everyone on the same page.

There Can Be Only One

The defining characteristic of a singleton class is that there is only one instance of it at any given time (unless it is never used, in which case, there may not be any instances of it depending on how it is implemented).

Implementing a Singleton is not quite as trivial as it sounds. The intuitive approach is to make the constructor private and provide a public static accessor to it, as shown in Figure 2.

Figure 2 Implementing a Singleton

public class MySingleton
{
    private static MySingleton _instance;
    private MySingleton()
    {
    }
    public static MySingleton Instance
    {
        get
        {
            if (_instance == null)
                _instance = new MySingleton();
            return _instance;
        } 
    }
    ...
}

This works fine in single-threaded environments, but not in multi-threaded ones. Two threads could enter the Instance property before the instance variable is instantiated, resulting in two instances of the class. There are a few ways around this,  but the easiest solution in .NET takes advantage of the CLR and its guarantees around initialization of static variables, as shown in Figure 3.

Figure 3 Dealing With Two Instances of the Class

public class MySingleton
{
    private static readonly MySingleton _instance = new MySingleton();
    private MySingleton()
    {
    }

    public static MySingleton Instance
    {
        get
        {
            return _instance;
        }
    }
    ...
}

The fact that the instance variable is marked static and readonly ensures only one instance can be created, even in multi-threaded environments.  Another side effect is that an instance of MySingleton is always going to be created for the _instance variable, regardless of if it’s needed or not.  This is, however, rarely an issue in real world code.

Some Background

A common implementation for singletons is to use double-checked locking, like so:

if (_instance == null) {
    lock (_initializationLock) {
        if (_instance == null) {
            _instance = new MySingleton();
        }                    
    }                
}
return _instance;

On the surface, double-checked locking appears to offer the best of all worlds. Singletons are instantiated lazily, are never instantiated if never used, and do not take an expensive lock after the singleton is initialized. Unfortunately, double-checked locking is subtlely broken in the vast majority of cases. You can read more about the problems in The “Double-Checked Locking is Broken” Declaration by David Bacon, et al. Double-checked locking can be fixed on the CLR (and JDK5+) by declaring the _instance variable with the volatile keyword. See Implementing Singleton in C# and Exploring the Singleton Design Pattern in the MSDN Library for more information.

That’s about as detailed as we’re going to get on implementing singletons, because there are a number of good resources already available. Plus, the focus of this article is to move away from them, not add more.

With that background behind us, the “singleton” classes in ScrewTurn aren’t actually singletons. Figure 4 looks at the AuthChecker class first.

Figure 4 AuthChecker Class

public class AuthChecker {
    private static AuthChecker instance;
    public static AuthChecker Instance 
    {
        get { return instance; }
        set { instance = value; }
    }

    private ISettingsStorageProviderV30 settingsProvider; 
    public AuthChecker(ISettingsStorageProviderV30 settingsProvider) 
    {
        if(settingsProvider == null) throw new 
            ArgumentNullException("settingsProvider");
        this.settingsProvider = settingsProvider;
    }

    public bool CheckActionForGlobals(string action, 
        string currentUser, string[] groups) 
    {
    }

    public bool CheckActionForNamespace(NamespaceInfo nspace, 
        string action, string currentUser, 
        string[] groups) 
    {
    }

    public bool CheckActionForPage(PageInfo page, string action, 
    {
    }

    public bool CheckActionForDirectory(
        IFilesStorageProviderV30 provider, 
        string directory, string action, 
        string currentUser, string[] groups) 
    {
    }
}

The most obvious way you can tell this isn’t a singleton is that it has a public constructor. So the client code can create as many of these as it likes. Furthermore, the class doesn’t actually instantiate the instance variable itself. That is done elsewhere in the code, in StartupTools.cs, using the public constructor.

That said, it is still used as a singleton within the context of the application. Yes, the code could create more than one instance and re-assign the existing instance and basically treat it like any other class. But in the application’s current state, it doesn’t. This half-way status between singleton and non-singleton would be a bone of contention for us at a code review, so let’s see what we can do to fix it. But first, like any refactoring, we need to justify it.

He’s Liked, But He’s Not Well Liked

If the current code works, why do we need to get rid of the singleton? What harm is it doing? After all, there’s a reason the pattern has a name. It must be useful.

We have a few issues with singletons. The easiest argument to make is that singletons make your code hard to test. If you make a call to a singleton in a class, then you must make sure the singleton is initialized whenever the test is run. If the singleton requires a connection to a database or the existence of a Web service, these must be configured prior to running the test. If you want reasons why this is a bad practice, look no further than our previous article on separation of concerns.

This lack of testability isn’t the only problem. After all, there are ways (usually neither easy nor pleasant) to make singletons testable. Indeed, ScrewTurn uses one of them by exposing a public constructor. Let’s look more closely at the AuthChecker class we’ve been working with. Why is it a singleton class? Perhaps so that its members can be accessed throughout the site without having to create a new instance of it? Perhaps to save on resources by reducing object allocation and garbage collection? (This would almost certainly be a premature optimization.)

Let’s look at the first public method on the class, CheckActionForGlobals. Where is this being used? If you trace its usages, you’ll find it is used in exactly three other classes: the AdminMaster master page and FileManager.ascx control (both in the WebApplication project) and the AuthorizationServices class in the Core project. (The latter is one of the refactorings we did in Part 6; originally, it was being called in Default.aspx in the WebApplication project.)

So this globally accessible method is really only being used in three places. Doesn’t sound all that global to us. One could make an argument that each of these classes could just create an instance of the class instead. That would make them easier to test and less fragile overall.

Of course, we’ve been cautiously selective in our example. CheckActionForGlobals is but one method in the AuthChecker singleton. If we look at all cases where we reference AuthChecker.Instance, we’ll find no less than 86 occurrences of it across 21 files, as shown in Figure 5.

Figure 5 All References to AuthChecker.Instance

“Aha!” you say. “Doesn’t that contradict your argument that it’s not really being used globally?” “Pshaw!” say we! What it does is highlight another issue with singletons. The Instance variable is used in 21 files, yet CheckActionForGlobals is used in 3. Another method, CheckActionForDirectory, is used in 5, not including calls made from within the AuthChecker class itself.

In short, our singleton has turned into a sort of catch-all bucket, a place to house disparate methods under the loosely-defined category of “authorization checking.” Granted, the AuthChecker class isn’t as messy as it could be, but the tendency with singleton classes is to make them more and more generic as you add “helper” methods to them. The ubiquitous Utils singleton often seen in many projects is a good example. Developers see a singleton and it’s just so darned tempting to drop a method in there rather than create a separate class with a more specific focus.

The result of those temptations is that you find code bases that are littered with unnecessary singletons.  While they may seem innocuous at first, the problems we described earlier will start to haunt your project.  To help you with those existing Singletons, let’s look at how we can move through our code and remove them.

I’ve Got Some Seeds, Right Away

So where should we start?  How about the AuthChecker class, since that’s what we’ve discussed through the first part of this article.  Earlier, we decided that there was no reason for it to continue to be a singleton, since there was no compelling reason for one, and only one, instance of it to exist for the life of the application.

The process that we’re going to go through will see us move from using the existing AuthChecker class to using instances of an AuthorizationChecker class.  Once we’ve made the transition from AuthChecker to AuthorizationChecker, we will delete the AuthChecker Singleton from our codebase.

The first challenge that we have to face in this process is how do we create a new class (AuthorizationChecker) which we can guarantee to expose the same functionality as the original (AuthChecker) class.  There are two reasonable options for this: abstract base class and interface.  Abstract base class is a good solution, if we were going to need to use the same AuthChecker functionality in multiple class implementation for the distant future.  In this case though, we’ve already determined that we’re going to be deleting the AuthChecker Singleton implementation once we’ve completed our refactoring.  As a result, using Interfaces makes more sense.

That makes our first step in the process to extract an interface from the AuthChecker class.  As you can see below, it’s really a simple interface, called IAuthorizationChecker, that defines the four publically exposed methods on the AuthChecker class:

public interface IAuthenticationChecker
{
    bool CheckActionForGlobals(string action, string currentUser,
        string[] groups);
    bool CheckActionForNamespace(NamespaceInfo nspace, string action,
        string currentUser, string[] groups);
    bool CheckActionForPage(PageInfo page, string action,
        string currentUser, string[] groups);
    bool CheckActionForDirectory(IFilesStorageProviderV30 provider, 
        string directory, string action, string currentUser, string[] groups);
}

Now that we have that interface we need to get the AuthChecker to implement it.  This is pretty easy, but has a couple of small twists.  Because we’re applying the interface to a Singleton class, we also have to ensure that the .Instance property is returning a type of the IAuthenticationChecker interface.  To get the code to compile we also have to change the type of the properties underlying module level variable so that it too is of type IAuthorizationChecker:

public class AuthorizationChecker : IAuthorizationChecker
{
    public bool CheckActionForGlobals(string action, 
        string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }

    public bool CheckActionForNamespace(NamespaceInfo nspace, 
        string action, string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
 
    public bool CheckActionForPage(PageInfo page, string action, 
        string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
 
    public bool CheckActionForDirectory(
        IFilesStorageProviderV30 provider, string directory, 
        string action, string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
}

Interestingly, this is the last of the changes that you’ll have to make to AuthChecker before you delete it from the project.

Now that we’ve established how calling code should expect us to behave (via the IAuthorizationChecker interface), we can go ahead and write a new class that meets that expectation.  In this case we’re going to call the class AuthorizationChecker and we’ll make it implement the interface.  The result, as shown in Figure 6, is a class with the same four public methods on it that the AuthChecker class has.

Figure 6 Class With Four Public Methods

public class AuthorizationChecker : IAuthorizationChecker
{
    public bool CheckActionForGlobals(string action, 
        string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
 
    public bool CheckActionForNamespace(NamespaceInfo nspace, 
        string action, string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
 
    public bool CheckActionForPage(PageInfo page, string action, 
        string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
 
    public bool CheckActionForDirectory(
        IFilesStorageProviderV30 provider, 
        string directory, string action, 
        string currentUser, string[] groups)
    {
        throw new NotImplementedException();
    }
}

Now that our new AuthorizationChecker class has a basic shell, we need to get it working with the same functionality as the original AuthChecker singleton.   One of the key things when doing refactorings like this is to be able to do them incrementally.   Big-bang approaches can be successful, but rarely do you have the ability to impose the constraints, such as time and cost, on your project’s current needs.  Instead, we want to be able to nibble away at the refactoring as time permits and in a way that won’t affect the project.

To do this, we’re going to take our new AuthenticationChecker class and simply pass through the calls to the original AuthChecker code.   This may seem like an extraneous step, but it’s one that is necessary to achieve the goal of refactoring little by little.  To make this happen, we need to get an instance of AuthChecker into the local scope of the newly created AuthenticationChecker class:

private IAuthorizationChecker _authChecker;

public AuthorizationChecker()
{
    AuthChecker.Instance = new AuthChecker(Settings.Instance.Provider);
    _authChecker = AuthChecker.Instance;
}

Once we have the AuthChecker instance in place, we can have the methods in the class simply delegate execution to the existing methods in the AuthChecker class.  Like we said before, this may seem extraneous, but trust us, we’re going somewhere here, as shown in Figure 7.

Figure 7 Execution of Existing Methods in the AuthChecker Class

public bool CheckActionForGlobals(string action, string currentUser, 
    string[] groups)
{
    return _authChecker.CheckActionForGlobals(action, currentUser,
        groups);
}

public bool CheckActionForNamespace(NamespaceInfo nspace, 
    string action, string currentUser, string[] groups)
{
    return _authChecker.CheckActionForNamespace(nspace, action, 
        currentUser, groups);
}
 
public bool CheckActionForPage(PageInfo page, string action, 
    string currentUser, string[] groups)
{
    return _authChecker.CheckActionForPage(page, action, 
        currentUser, groups);
}

public bool CheckActionForDirectory(IFilesStorageProviderV30 provider, 
    string directory, string action, string currentUser, string[] groups)
{
    return _authChecker.CheckActionForDirectory(provider, directory, 
        action, currentUser, groups);
}

One of the key things to note with this approach is that we’ve maintained a single place of change for the code that is related to the AuthChecker/AuthorizationChecker functionality.  If we’re going to be tackling this refactoring over a period of time, this is key, since we can’t be confidently maintaining multiple pieces of code that represent the same functionality.  Don’t be fooled that the maintenance cost of this approach is zero.  If you were to change the arguments in the methods, you’d have multiple places to change, but, on a positive note, you’d get compile errors from that due to the use of an interface enforcing the publically exposed contract.

Now that we have a new class, complete with functionality, we can start to refactor other parts of the application to use it instead of the AuthChecker singleton.  Like all the other refactorings we’ve done thus far in this article, it’s not that big of a deal.  As our example, we’re going to continue to refactor the AuthorizationServices class (see the previous Separation of Concerns article for the initial refactoring for this).  In AuthorizationServices, we want to eliminate the use of AuthChecker and, in its place, use an instance of AuthorizationChecker instead.

To do this, we first need to create a module level variable that provides the same capabilities as the AuthChecker singleton.  Earlier in this article, we created an IAuthenticationChecker interface that worked to enforce this for us.  Now is another time that we can use it.  As you can see in the code below, we now have created an instance of the AuthenticationChecker and assigned it to the module level variable:

public class AuthorizationServices
{
    private IAuthorizationChecker _authenticationChecker;
    public AuthorizationServices()
    {
        authorizationChecker = new AuthorizationChecker();
    }
    ...
}

Now that we have a way to access the authentication checking code, we just need to replace the AuthChecker calls in the RetrieveCurrentStatusFor method with calls to the module level variable.  As an example of these changes, here is what the first one would look like:

CanView = _authorizationChecker.CheckActionForPage(currentPage, 
					Actions.ForPages.ReadPage, currentUsername, 
					currentGroups),

With that we have our AuthorizationServices class refactored and we’ve finished the first full round of changes that are required to eliminate the AuthChecker singleton.  If we take a look at the usages of the AuthChecker singleton instance, we’ll see, as Figure 8 shows, that there are still a large number of places that it is being used.

Figure 8 Usages of AuthChecker Singleton Instance

Now that you’ve finished this first complete refactoring to use the new AuthorizationChecker class, you’ve also reached the first logical checkpoint in your refactoring exercise.  From here you can choose when you want to move from the AuthChecker singleton to an AuthorizationChecker instance class in any of the other places that AuthChecker is being used.  As you incrementally move through your changes from AuthChecker to AuthorizationChecker, you get closer and closer to the next step in this refactoring: deleting the AuthChecker singleton.

Once you have all of the usages of AuthChecker replaced with the AuthorizationChecker class you’re almost ready to remove the AuthChecker class from your code base.  Before we can do that, remember that we had the new AuthorizationChecker class delegating execution to the AuthChecker singleton.  To be able to get rid of AuthChecker completely, we need to move its logic into the new AuthorizationChecker class.  There are a couple of small things that we need to address in this refactoring, such as adding using statements.  

The main change we need is to address the code’s need for an ISettingsStorageProviderV30 typed object.  If we look into how that object is being passed into the AuthChecker singleton, we see that it’s actually being provided from another singleton named Settings:

AuthChecker.Instance = new AuthChecker(Settings.Instance.Provider);

Since this value is coming from a singleton, we can easily add a module level variable to AuthorizationChecker that can be used throughout its methods.  Assigning that variable is as simple as what you see below.  Also note that we no longer need the module level AuthChecker variable since we’ve now moved all of the AuthChecker code into this class.  Since it’s not needed, we’ve removed it and the code related to it in the constructor:

private ISettingsStorageProviderV30 _settingsProvider;

public AuthorizationChecker()
{
    _settingsProvider = Settings.Instance.Provider;
}

The result of this final refactoring is that the public methods on AuthChecker are no longer being executed anywhere in the code base.  If you were to do a Find Usages on AuthChecker, you’ll see that the only place it exists is in the StartupTools class where it was being initialized.  Since it’s not being used anywhere, there’s no need to initialize it, so we’ll just delete that line of code.  Now we’re completely free of any use of AuthChecker, so we can delete the class from the project.  With that, you’re free of your singleton past...or at least one part of it.

I realized what a ridiculous lie my whole life has been...

We’ve spent some quality time together on this article and have weaned you from a dependence on singletons in your code base.  Singletons often are seen as the “easy” way to code.  Just because you don’t have to “new up” a class to make use of its functionality, doesn’t mean that you’re not imposing a debt on your code base.  While singletons seem like the route of least friction at first, you quickly find out that they’re nothing more than a mirage on the way to software development bliss.  You see the ease of development that they offer, but every time that you move closer the goal is still just as far away as it was before.

Unless you truly need to ensure that one, and only one, instance of an object exists per application lifecycle, then you should be working to refactor singletons out of your code base.  Hopefully, the techniques in this article have pointed you in the right direction to be able to do that.