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:

lock(items)
{
items.add(o);
}

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();

lock(listLock)
{
items.add(o);
}
}

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.