What’s wrong with this code #6? (Discussion)

Thanks for all the comments on my recent post. I have one more that I’ll do in the next couple of weeks, and after that, I’m looking for suggestions for future posts. If you have an idea for one, please email me.

The answer I was looking for was related to the object that I was locking on, and Scott Williams was the first to get the answer that I was looking for – changing the code to:


The original code is bad because it can result in a deadlock – if somebody locks on a Notifier instance on one thread, and then does something that calls Notifier.Add() on another thread. Not really that likely in my book.

So, that’s one reason not to do it. A more compelling reason to lock on items is that, for best performance, you should lock on as granular an object as possible, and items is more granular than the entire Notification instance. If you have two lists that are both being protected by a single lock, that may slow things down.

Now, onto the comments. I’ve elected not to comment on a few cases where I didn’t think I could do it nicely. If you aren’t mentioned below and you want to know what I thought, feel free to email me.

Nick suggested locking on the SyncRoot property of the collection. This will work, but I don’t see any advantage to using this over just using “items” in this case. Brad wrote a bit about this very subject.

TAG went on a bit of a tangent in his discussion. It is true that you can use the Synchronized wrapper around an ArrayList(), but I think it’s generally better to do the wrapping yourself.  Not only does that give you better control over how the locking happens, it makes it obvious to the reader that you are doing synchronization. If you want/need to encapsulate, then do so in a well-named class.

I don’t think that you should expose a property that allows others to lock your object. Locking is something private to the object, and encouraging other objects to do locking for you is a bad thing. Or, as another commenter (replier? poster?) noted, “hilarity does not ensue”.

Drew commented that the .NET library design guidelines say you shouldn’t lock in instance methods of a library. I think the third bullet point is really more of an explanation for why the .NET libraries aren’t threadsafe rather than a general guideline. If you’re building a general-purpose library, then I agree with the guideline. If I’m building something to be used within my application, then I don’t agree – I would rather build it into the library layer – where it is often more straightforward to write and easier to verify – than add it on on the application layer. I think it’s more robust that way.

I do agree that over-zealous synchronization can kill perf.

In this case, though I didn’t mention this, this was application-level code.

Finally, Filipe suggested declaring a static object variable, and locking on that. Something like:

class Notifier
   static object listLock = new object();


This will work fine. Unfortunately, it provides one lock for all instances of the Notifier method, so that threads who are using different instances will still have to wait. In other words, the lock object isn’t at the same granularity as the resource we want to protect.

This is what you want to do if you need to protect a static resource and you don’t have a handy instance to lock on.

Comments (3)

  1. MattHope says:

    Interesting – I hadn’t seen the Blog on SyncRoot being deprecated.

    Could someone at MSFT do this for me (and any others) trying to stay abreast of developments but without the time to read (or indeed find) every blog.

    List of things we got wrong or "’X’ considered harmful"


    "Extending ApplicationException considered Harmful"

    "Locking on SyncRoot considered Harmful"

    Along with what you suggest we *should* do instead.

    This should be on MSDN with (preferabbly both) an RSS/Atom feed…

    Feel free to let the marketing types call it "Best Practice guidelines" or whatever but centralise this information…

  2. BTJ says:

    What about this:

    class Notifier


    private object listLock = new object();






    I’ve seen this suggested a few places for protecting instance state. Thoughts?

  3. TAG says:

    Exposing lock object has nothing wrong if done carefully.

    Consider example if some other part of your code need to Add 2 elements in Notifier collection only in case if some other object is already registered.

    There is no one method inside your collection – you have to call Contains() and 2 Add() – all of them will be atomic alone – but you need entire!! operation to be done atomically.

    The only good way for this is to expose lock object you already use to adding/removing items.

    But I agree – if possible – you must make application non-thread safe by default – but think about threadsafety and provide some way to make it safe.

    Using Synchronized wrapper is matter of your code readability and safety. It’s pretty easy to miss locks on some methods. Using Synchronized allow you to feel some kind of safety. I’ve clearly indicated that costs this have.

    BTW, using custom Synchronized wrapper can sometimes avoid useless locks. For example Hashtable.Synchronized takes advantage of internal algorithm design and allows multiple readers – single writer.

    P.S> There is no absolutely correct code sniplet – as different problems can requere different solutions.