Thread Safety Via Read Only Collections

0 comments suggest edit

UPDATE: Made some corrections to the discussion of ReadOnlyCollection’s interface implementations near the bottom. Thanks to Thomas Freudenberg and Damien Guard for pointing out the discrepancy.

In a recent post I warned against needlessly using double check locking for static members such as a Singleton. By using a static initializer, the creation of your Singleton member is thread safe. However the story does not end there.

One common scenario I often run into is having what is effectively a Singleton collection. For example, suppose you want to expose a collection of all fifty states. This should never change, so you might do something like this.

public static class StateHelper
{
  private static readonly IList<State> _states = GetAllStates();

  public static IList<State> States
  {
    get
    {
      return _states;
    }
  }

  private static IList<State> GetAllStates()
  {
    IList<State> states = new List<State>();
    states.Add(new State("Alabama"));
    states.Add(new State("Alaska"));
    //...
    states.Add(new State("Wyoming"));
    return states;
  }
}

While this code works just fine, there is potential for a subtle bug to be introduced in using this class. Do you see it?

The problem with this code is that any thread could potentially alter this collection like so:

StateHelper.States.Add(new State("Confusion"));

This is bad for a couple of reasons. First, we intend that this collection be read-only. Second, since multiple threads can access this collection at the same time, we can run into thread contention issues.

The design of this class does not express the intent that this collection is meant to be read-only. Sure, we used the readonly keyword on the private static member, but that means the variable reference is read only. The actual collection the reference points to can still be modified.

The solution is to use the generic ReadOnlyCollection<T> class. Here is an updated version of the above class.

public static class StateHelper
{
  private static ReadOnlyCollection<State> _states = GetAllStates();

  public static IList<State> States
  {
    get
    {
      return _states;
    }
  }

  private static ReadOnlyCollection<State> GetAllStates()
  {
    IList<State> states = new List<State>();
    states.Add(new State("Alabama"));
    states.Add(new State("Alaska"));
    //...
    states.Add(new State("Wyoming"));
    return new ReadOnlyCollection<State>(states);
  }
}

Now, not only is our intention expressed, but it is enforced.

Notice that In the above example, the static States property still returns a reference of type IList<State> instead of returning a reference of type ReadOnlyCollection<State>.

This is a concrete example of the Decorator Pattern at work. The ReadOnlyCollection<T> is a decorator to the IList<T> class. It implements the IList<T> interface and takes in an existing collection as a parameter in its contstructor.

In this case, if I had any client code already making use of the States property, I would not have to recompile that code.

One drawback to this approach is that interface IList<T> contains an Insert method. Thus the developer using this code can attempt to add a State, which will cause a runtime error.

If this was a brand new class, I would probably make the return type of the States property ReadOnlyCollection<State> which explicitly implements the IList<T> and ICollection<T> interfaces, thus hiding the Add and Insert methods (unless of course you explicitly cast it to one of those interfaces). That way the intent of being a read-only collection is very clear, as there is no way (in general usage) to even attempt to add another state to the collection.

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

14 responses

  1. Avatar for Damien Guard
    Damien Guard March 25th, 2007

    "If this was a brand new class, I would probably make the return type of the States property ReadOnlyCollection<State> which does not contain an Add method."
    ReadOnlyCollection<T> does contain an Add method - if it didn't then it couldn't be a decorator to IList<T> which defines the Add method.
    It is a bad example of the Decorator design pattern in that it does not "Attach additional responsibilities to an object dynamically" but instead removes responsibility by failing to implement the majority of the IList/ICollection interface - something which should be ringing alarm bells.
    Personally I'd go with defining a StateList class that just implemented IEnumerable<State> and provided Count, Contains, IndexOf and Item[] members.
    [)amien

  2. Avatar for Haacked
    Haacked March 25th, 2007

    Actually, ReadOnlyCollection<T> does not contain the Add method.
    Check out the MSDN documentation. Here's a list of all members of ReadOnlyCollection<T>.
    Not to mention that IList<T> does not contain the Add method either.
    It does implement the Insert method though, which throws a runtime exception, which is a point I overlooked since I rarely use Insert when populating a collection.
    I still think it's a fine example of a decorator in that it added a responsibility that I didn't already have with my IList implementation. It made it read-only.
    In this context, since that's a feature I want, and I already committed to the IList<T> interface, I think it's still fine to call it a decorator.

  3. Avatar for Thomas Freudenberg
    Thomas Freudenberg March 25th, 2007

    ReadOnlyCollection<T> inherits ICollection<T>, so it has to implement ICollection<T>.Add. However, ReadOnlyCollection<T> hides the method by implementing it explicitly and throwing a NotSupportedException.

  4. Avatar for CodeObsessed
    CodeObsessed March 25th, 2007

    (this is currently a placeholder article which will be updated later) Looking at Phil Haack's article

  5. Avatar for AlSki
    AlSki March 25th, 2007

    However a bigger question is should we be restricting the users of our class in this way. Do we wish to empower the users of our class with the ability to change things, (e.g. "Alaska splits into 50th and 51st State in war over the list of correct names for snow") or simply lock them in.
    I think the fact that we want to do this, is actually more revealing of our attitude to our fellow developers and their abilities, and its something I constantly battle with. See
    http://www.martinfowler.com...

  6. Avatar for Bill Pierce
    Bill Pierce March 25th, 2007

    If you want to get really hard core read only uber-cool, just expose an IEnumerable. You can still use it for databinding and enumeration, don't have to worry about people even trying to modify the contents. If the user wants to, they can use it to instantiate their own collection and have their way with it.

  7. Avatar for Haacked
    Haacked March 25th, 2007

    @Alski - That's a good question. I actually have a follow-on blog post to address that topic.
    Adding a US state happens so rarely, that if it did happen, I think it's fine to restart the application in order to reload that collection. In fact, since most of my focus is on web apps, that's not such a big deal.
    As for creating "restrictions", it's a matter of perspective. I don't see this as restricting other developers. I see it as signalling my intent.
    The fact that there's no Add method lets other devs know that I didn't intend for this collection to be added to and doing so could cause unforeseen problems.
    Another example is using the ref keyword in a method when you mean out. In the IL, they're the same, but in C# they signal slightly different intents, which is useful to know when reading the code.

  8. Avatar for Haacked
    Haacked March 25th, 2007

    @Bill - True. For my second scenario where I get free reign to rewrite the class.
    However, in the first scenario, that wouldn't work. If I've already exposed the class to client code as an IList<T>, I can't just change the interface.
    Also, users of the property lose the ability to reference a state by index! Quick, what's the 49th state alphabetically?
    Console.WriteLine(StateHelper.States[48]);
    ;)

  9. Avatar for Haacked
    Haacked March 25th, 2007

    @Thomas, thanks for the clarification! I will note it in the post.

  10. Avatar for Bill Pierce
    Bill Pierce March 25th, 2007

    Phil,
    You've been quite the minutiae maniac lately. So I thought I'd strike back :)
    "Quick, what's the 49th state alphabetically?"
    You make no guarantee to me via your API that the States have been loaded into the collection alphabetically. So I would have to sort your list myself so I'd have to create my own list/array because I can't sort your list in place because it's read-only!
    Your move my friend.

  11. Avatar for Haacked
    Haacked March 25th, 2007

    Heh heh. Point well taken Bill! You've exposed my hand.
    In the implementation of GetAllStates above, I was intending to list the states in alphabetical order. For brevity, I only listed three. The first two, ellipses, the last one.
    However all that was in my head. I didn't make that clear. Perhaps I should create a ReadOnlyAlphabeticalCollection. ;)
    Even if it wasn't a read only collection, you would want to copy it to a local collection/array and sort it. Otherwise you'd have to lock it before sorting since another thread might be sorting it at the same time in a different manner.

  12. Avatar for Haacked
    Haacked March 25th, 2007

    @Bill, I forgot to add. "Check!"

  13. Avatar for Damien Guard
    Damien Guard March 26th, 2007

    Yes, it's always worth remembering that an interface can inherit from another interface.
    IList inherits from ICollection and so the IList interface does contain an Add method even thought it is inherited and not explicitly defined there.
    [)amien

  14. Avatar for Haacked
    Haacked March 26th, 2007

    @Damien: True. I've fired my fact checkers and hired new ones.
    ;)