MVC: Locking the RouteCollection

Since the advent of multithreaded programming, the responsibility of locking collections has always been a contentious issue. Who should lock the collection? When should it be locked? And for how long? The ASP.NET Routing feature that is used by the MVC framework involves a thread-safe collection that contains the list of the application's route definitions. Will your routes be safe? Continue reading to find out (on this amazing journey (into the depths of the RouteCollection)). How's that for a cheesy intro?

The problem

When the first preview of ASP.NET MVC was released last December, some people noted that the RouteCollection would lock itself during all read and write operations, which could cause major performance and scaling issues. Well, sort of. The RouteCollection class derives from the generic Collection<T>, which has a few virtual methods it calls when write operations are performed. Each of those methods was overridden in RouteCollection to take a full lock. The route operations of matching routes to incoming requests and generating URLs also took the same full lock. There were two problems (at least) with this approach:

  1. Taking a full lock means that if anyone is using the collection, any other operation on that collection would have to wait for the first one to be done. Even if two requests just wanted to enumerate the collection (a read-only operation), one request would have to wait. This wasn't a logic bug since it wouldn't prevent anything from working; It was just a performance issue. There was in fact a comment in the code along the lines of "TODO: Shouldn't we use a ReaderWriterLockSlim here or something?"
  2. We weren't locking enough! If someone directly enumerated the collection by calling GetEnumerator (which happens implicitly when you use C# or VB's foreach statement), no lock would be taken. Unfortunately, Collection<T> does not offer any virtual methods that get called during read operations.

The solution (or so we thought)

We knew we had to do something for ASP.NET MVC Preview 2: We had performance problems as well as bugs in our collection locking. The first thing we did was change the lock to be a multiple-reader, single-writer lock with the ReaderWriterLockSlim class. Using this class in the RouteCollection allows any number of readers to read the collection at the same time but will fully lock the collection when a write is performed and allow only that one writer to make changes until the write lock is released.

Before we knew it, though, a bug was found while running some performance tests. When there is contention for the lock, the ReaderWriterLockSlim needs to get the processor count of the server. For some reason or another, getting that count is not permitted in Medium Trust web applications and thus throws a SecurityException. We didn't notice this while we were writing unit tests and web tests because all those tests are single threaded: there is never any contention for the lock.

The final solution

We opened a work item for the owners of Environment.ProcessorCount to remove the security requirement since knowing the processor count of the host is hardly top secret information. However, since the ASP.NET Routing feature works on .NET Framework 3.5, bug fixes in a future version wouldn't be sufficient. Thus we changed RouteCollection to use ReaderWriterLock, which is slightly less performant, but works in all web trust levels.

So we solved the problem of when the collection gets locked and for how long. But what about the very first issue I mentioned: Who should lock the collection?

Our first solution was to do all the locking internally. Any time you called any member on the collection we'd take the appropriate lock. This meant implementing IList<T> directly since neither Collection<T> nor List<T> provide the necessary hooks. This proved to be a lot of work (but not for me, since I didn't have to write it smile_regular) and left some big holes of functionality. For example, how could someone atomically search for a route in the collection, remove it, and add a new route?

The solution to that problem was to expose the lock semantics publicly, though without exposing the lock object itself. Thus the RouteCollection has two new methods: GetReadLock and GetWriteLock. Developers can call those methods to get the lock that the RouteCollection uses internally and cooperate with its locking mechanism. However, you only need to get the lock if you're directly accessing the collection. If you're going through any of the typical "end-user" API calls, we do the locking automatically. For example, calling Html.ActionLink, Url.Action, and RouteCollection.GetVirtualPath will automatically lock the collection.

In case you're wondering, there were several reasons we chose not to expose the lock object itself:

  1. We wanted to be able to change the lock implementation at any time. This would let us switch back to ReaderWriterLockSlim at some point in the future.
  2. It seemed simpler for users to have a limited API surface. Less is More.
  3. It encourages users to use the dispose pattern with the lock they get, thus ensuring that the lock is released even if an exception is thrown and not caught while the lock is being held.

Why you should care

There are two takeaways I hope I provided in this article:

The first takeaway is that if you're implementing a thread-safe collection you have some food for thought. I hope the food was easy to digest!

The second takeaway is that if you're directly accessing the RouteCollection in an ASP.NET application, make sure you take the lock first! Taking the lock is easy with the dispose pattern:

RouteCollection routes = RouteTable.Routes;
using (routes.GetReadLock()) {
    foreach (RouteBase route in routes) {
        // Do something interesting here and be guaranteed that no one is modifying the collection
    }
}
// The lock is now released since we exited the "using" statement.

Who would have thought that a simple collection of routes would turn out to be so much fun? Did you ever start coding something that seemed simple and ended up writing a long blog post about it?

5 Comments

  • Shame you can't use ReaderWriterLockSlim. I would find that erking :/

    I like your explanation of exposing lock semantics instead of the lock itself. I've done something similar once by returning ActionDisposable:IDisposable on AcquireReaderLock(). It's a sweet api technique.

    The slim RWLock has the concept of upgradable locks. I suggest doing this when you use RWLockSLim in the future - although it's an extra API call. Maybe make read locks are automatically upgradable. What would u have done in this case?



  • Very interesting about ReaderWriterLockSlim... I was going to bash you guys for not using it, but this makes perfect sense.

    It really is nice to the team taking input from the community and making good decisions with it.

    +1

  • Isn't performing multiple operations atomically on a collection the point behind the SyncRoot property on the ICollection interface (which was dropped for some reason in ICollection)? Why did you choose to use the GetReadLock method, instead of exposing a SyncRoot, which would allow developers to use lock/SyncLock functionality built into their chosen language?

  • Hi David,
    The problem with SyncRoot is that it doesn't let you do fancier kinds of locks - such as the multiple reader, single writer lock that we use now. With the SyncRoot you could only take a full lock using the lock(root) statement in C#.

    By hiding the locking mechanism we are also free to change the precise implementation. As I mentioned in the blog post, we could change it to use the more efficient ReaderWriterLockSlim in a future version.

    Thanks,
    Eilon

  • I have a thread safe dictionary that uses ReaderWriterLockSlim. I decided your method made things more explicit, so I set my dictionary to be "thread-unsafe", added the GetXXLock() methods and ran it in a test method. Here's the test method:

    [TestMethod]
    public void TestSafeDictionary()
    {
    Random rand = new Random(1000);
    ISafeDictionary dict =
    new SafeDictionary();
    dict.SetSafety(ThreadSafeType.None);
    for (int i = 0; i
    {
    using (dict.GetReadLock())
    {
    foreach (object item in ((IEnumerable)o))
    Thread.SpinWait(10000);
    }
    });
    Thread tmutate = new Thread(
    o =>
    {
    using (dict.GetWriteLock())
    {
    IDictionary d = o as IDictionary;
    string randKey = rand.Next().ToString();
    if (!d.ContainsKey(randKey))
    d.Add(randKey, randKey);
    }
    });
    Thread tenumkeys = new Thread(
    o =>
    {
    using (dict.GetReadLock())
    {
    foreach (string key in ((IDictionary)o).Keys)
    Thread.SpinWait(10000);
    }
    });
    Thread tenumvals = new Thread(
    o =>
    {
    using (dict.GetReadLock())
    {
    foreach (string key in ((IDictionary)o).Values)
    Thread.SpinWait(10000);
    }
    });
    tenum.Start(dict);
    tmutate.Start(dict);
    tenumkeys.Start(dict);
    tenumvals.Start(dict);
    }
    }

    This test resulted in a massive cascade of exceptions, all the same type:

    "One of the background threads threw exception: System.Threading.SynchronizationLockException: The lock is being disposed while still being used. It either is being held by a thread and/or has active waiters waiting to acquire the lock.
    at System.Threading.ReaderWriterLockSlim.Dispose(Boolean disposing)
    at System.Threading.ReaderWriterLockSlim.Dispose()"

    So it appears the using() statement will be a non-starter in a true multi-threaded environment.

Comments have been disabled for this content.