Add/Remove items thread-safely in List<T>

Question

Recently I had to lock collections (of type List<T>) when items were added or removed. Because several collections were used in given code instead of creating helper methods for each collection, I made an extension methods:

public static class MyExtension
{
    public static void AddThreadSafely<T>(this ICollection<T> collection, T item)
    {
        if (collection == null)
            throw new ArgumentNullException("collection");

        bool locked = false;
        try
        {
             System.Threading.Monitor.Enter(collection, ref locked);
             collection.Add(item);
        }
        finally
        {
             if (locked)
                 System.Threading.Monitor.Exit(collection);
        }
    }

    //The same with remove
}

and then simply replaced Add with AddThreadSafely.

I like the simplicity of this solution but... What are the drawbacks of using this method? Will it be good idea to use this method of locking collections in future projects?

Answer 1

I would advise against this approach, you are correct in the fact that it is simple, however it's not foolproof. There is nothing actually ensuring that someone doesn't just call .Add() on the collection bypassing your extension method which allows for difficult to find bugs/race conditions.

Don't forget that all Monitor.Enter does is ensure that if multiple threads call Monitor.Enter that only 1 can enter a code block at a time. It doesn't prevent a caller on another thread mutating the collection if they don't call Monitor.Enter on the same obejct.

You would be better off either creating a custom collection which inherits from CollectionBase and encapsulating the lock logic in there so that any caller calling .Add() goes through the lock and is unable to bypass it.

Or if you are using .NET 4.0+, look at one of the collections in System.Collections.Concurrent as they are designed for this sort of activity in the first place.

Answer 2

EDIT: +1 for throwing on null in extension method. That's the right way to go.

1. Use BlockingCollection (starting .Net 4) - that's what it is for.
2. Not sure you need that flag. Why don't Monitor.Enter(collection), and always Monitor.Exit() on the finally block?