Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

2 Answers 2

up vote 18 down vote accepted

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.

share|improve this answer

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?
share|improve this answer
2  
1. Thanks for BlockingCollection. 2. If collection is null extension method will be called because extension methods don't make virtual call (uses call il instruction, not callvirt). 4. If there were no exceptions thrown means that item was added. So, IMO, returning boolean is odd here. Thanks for idea, BTW. –  Leri Nov 12 '12 at 11:07
    
I need that flag because compiler generates no-op instruction between Enter and try-finally so if lock is added and then happens exception before try block, you will never release lock so you get deadlock. –  Leri Nov 12 '12 at 12:06
3  
@PLB Instead of the whole try/catch, you should have just used lock. On .Net 4+, it compiles to exactly the same IL, with less written code and better compatibility (that overload of Monitor.Enter() doesn't exist in .Net 3.5). –  svick Nov 12 '12 at 14:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.