What's wrong with this code - #6

After a long hiatus, here's the next entry in my not-really-regular-enough-to-honestly-be-called-periodic-(though-I-try-to-keep-up-a-somewhat-reasonable-schedule-I-don't-seem-to-keep-it-since-I'm-lazy-are-you-still-reading-this)-series about C# code.

Anyway, here's some code that I've seen a fair bit, and written myself:

 class Notifier

{

   ArrayList items = new ArrayList();

   public void Add(object o) {

      lock (this) {

         items.Add(o);

      }

   }

}

This code will work fine, but it has a latent issue. What is the issue, and how should it be addressed?

Comments

• Anonymous
  April 22, 2005
  You shouldn't be locking on this, rather, you should be locking on the object exposed by the SyncRoot property on the ArrayList.
• Anonymous
  April 22, 2005
  Would it be better to just lock the items object, since that is all that is used in the lock statement?
• Anonymous
  April 22, 2005
  The only thing that I can think of - (apart from the bracketing style :))
  
  Is that we are locking an object (this) that is external - a caller could have locked it in which case we have a deadlock.
  
  I'd suggest
  lock(items)
  items.Add(o);
• Anonymous
  April 22, 2005
  If you lock on this then someone else can lock on you, and that's pretty bad...
  You should lock on a new private object in your class or in the SinkRoot of the ArrayList...
• Anonymous
  April 22, 2005
  Calling lock(this) can cause deadlocks. It would be recommended to have the Notifier class hold a dummy object that it can lock on or lock specifically on the object you are sharing across threads.
• Anonymous
  April 22, 2005
  I am guessing the problem is the locking itself, by locking "this" you are locking the entire class and not just the collection.
  
  When you do answer this can you explain why you would even want to lock the array list. I know it is to make it thread safe so multiple threads can't change it, but wouldn't it be better to just use the syncronized method? I guess I always thought of an ArrayList as a collection with a Changed event handler. This would invalidate any enumerations if something changed while another thread was looping through it. However upon looking at the array list further I see it doesn't have a public handler. Does it have a private one? I am thinking it should be like any other collection where if one thread changes it while another is looping it will throw. But I am not sure on the ArrayList I do not use it that much. Hmm off to ILDASM the ArrayList.
• Anonymous
  April 22, 2005
  The comment has been removed
• Anonymous
  April 22, 2005
  Definitely locking on "this" opens up the potential for a deadlock, but I wouldn't lock on SyncRoot either since it's a public property and thus could suffer from the same latent bug that locking on "this" would have.
  
  Ideally you should lock on a private variable. In this case, items seems the logical choice.
• Anonymous
  April 22, 2005
  The comment has been removed
• Anonymous
  April 22, 2005
  The comment has been removed
• Anonymous
  April 22, 2005
  My instinct says that the latent issue is caused by the fact that the instance methods of an ArrayList aren't thread safe. Sure, you've syncrhonized the Add, but enumerations and removes aren't protected.
  
  If I'm right, the solution would be to use the Synchronized version of the ArrayList. So the first declaration would be changed to ArrayList items = ArrayList.Synchronized(new ArrayList());
• Anonymous
  April 22, 2005
  As noted in the .NET library design guidlines[1], you shouldn't be locking at all in instance methods of a library. The application layer is the only place where the true needs of thread interaction are ever really known. Therefore building your libraries to deal with synchronization is a sure fire way to guarentee they will never perform well for applications who are only ever using the library in a safe, single threaded fashion.
  
  [1] http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpgenref/html/cpconthreadingdesignguidelines.asp
• Anonymous
  April 22, 2005
  Notifier lockMe = new Notifier();
  lock(lockMe)
  {
  lockMe.Add(new object());
  }
  
  Your internal locking object should not be accessable outside of your class, using the this reference makes it public to anyone. hilarity does not ensue. :|
• Anonymous
  April 22, 2005
  I don't think lock(this) is bad thing here, though it is not good practice in general. Notifier is internal class and doesn't implement any interfaces, thus it's unlikely, that it will be published to user. It may be returned as object, but this would be strange. Though, I would fix this as well.
  
  Another issue besides locking is that Notifier.Add doesn't return index of added element, so if user will need it, it will have to use IndexOf and slow down performance. So, it should have been rewrite as
  
  public int Add(object o)
  {
  lock(items) { return items.Add(o); }
  }
  
  One more thing I can think of is type-safety. I think that Add should be typed, but this depends on the context.
• Anonymous
  April 22, 2005
  The comment has been removed
• Anonymous
  April 22, 2005
  The comment has been removed
• Anonymous
  April 22, 2005
  items is a public object and you're trying to lock down the Notifier object so that only one thing can be added at a time. However, because items is public and not private you can't guaruntee that.
  
  Or this could be the reason why it's wrong:
  
  The expression of a lock statement must denote a value of a reference-type. No implicit boxing conversion (Section 6.1.5) is ever performed for the expression of a lock statement, and thus it is a compile-time error for the expression to denote a value of a value-type.
  
  From the help search
• Anonymous
  April 24, 2005
  orangy,
  
  Thanks for your comment. I've never thinked about this.
  Exposing internal data must be done carefully.
  
  Secure coding is not easy and very often requere code to be a little bit complicated.
  
  BTW, Exposing any object for locking will be unsafe - as some evil threads can do Monitor.TryEnter() on everything it see ;-)
  
  Regarding your comments - It's very sad that there is no ArrayList.Synhronized(ArrayList array, object lockObject) methods. (Same problem in Java - two arguments contructor for Collection taking lock object is package private).
  
  You have to create own wrapper to make custom synhronization.
  
  With your suggestion last code can (but it unable to) look like
  
  class Notifier
  {
  public object SyncRoot { get { return items.SyncRoot; }}
  
  readonly IList items = ArrayList.Synchronized(new ArrayList(), new object()); // Sync arraylist using supplied object as lock gate
  
  public void Add(object o) {
  items.Add(o);
  }
  }
• Anonymous
  April 24, 2005
  We should declare a static object variable, and lock the static object variable, not the this reference.
• Anonymous
  April 24, 2005
  class Notifier
  
  {
  ArrayList items = new ArrayList();
  public void Add(object o) {
  lock (items.SyncRoot) {
  items.Add(o);
  }
  }
  }
• Anonymous
  April 27, 2005
  The comment has been removed