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

  1. You shouldn’t be locking on this, rather, you should be locking on the object exposed by the SyncRoot property on the ArrayList.

  2. Would it be better to just lock the items object, since that is all that is used in the lock statement?

  3. Darren Oakey says:

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

  4. Juan Felipe Machado says:

    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…

  5. Nick Parker says:

    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.

  6. Jeff Parker says:

    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.

  7. Jeff Parker says:

    Hmm even more interesting if I read this correctly. Instead of an event handler to invalidate the framework compair a private variable called _version which is int32. Which oddly enough means that an Array list Since the version number always increases in like the Add method and the remove at method you could potentially over run since the version is started at 0 and will at least increase until Int32.MaxValue however I am suprised that somethings do not increase the version number like TrimToSize and Only one Sort method changes the version number and only one reverse overload increases the version.

    Hmm I guess I never really looked that deeply into an array list. So now why would it behave like this and not use a delegate to invalidate the collection. This would allow your Array list to be added and removed to as often as you want to. Now I am really intrigued I need to test out enough Adds and removes and sorts to crank up an array list version and see if i can get it to crash and overflow with only about 10 items in it.

  8. Haacked says:

    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.

  9. TAG says:

    In Java it’s recomended to use interfaces (if possible – not always) like a IList or ICollection instead of ArrayList in members declaration.

    http://www.javapractices.com/Topic26.cjp

    This will allow you to change actual storage type easily.

    But this has a runtime costs – as no function inlining possible.

    Not in this case – as ArrayList implementation anyway virtual – but in generics collection this rule will not apply.

    This make Java and C# different – as C# members by default are not virtual.

    Somethat that not good – there is no way to tell that your member does implement more then one interface.

    Like a IList and ICloneable in same time. This very often force me to use concrete classes instead of interfaces.

    Regarding locking on this – I feel that did this intentionaly.

    All suggestions to use lock(items) or lock(items.SyncRoot) are useless and have nothing to do with deadlocks.

    Locking on any object can lead to deadlock in the same way.

    If you was willing to perform atomic add/remove operations – you will use

    items = ArrayList.Synchronized(new ArrayList());

    But this is lame thread-safety – as if you have Add() – you probably will have others methods and

    users will be willing to obtain lock for duration of more then one method.

    If so – it’s recomended (as Nick Parker and Haacked figured out) to create new object and expose it as property.

    class Notifier

    {

    readonly object lockMe = new object();

    public object LockMe { get { return lockMe; }}

    readonly IList items = new ArrayList();

    public void Add(object o) {

    lock (LockMe) {

    items.Add(o);

    }

    }

    }

    One more issue – are you sure that you want to add null references to your Notifier items list ?

    And final one – are you going to change or null-out object used as items member ? If not – mark it as ‘readonly’.

    P.S> Sorry. Can not stop 😉 Use better names for variable o. It’s easy to confuse with 0. Name like obj or item will be good.

  10. TAG says:

    P.P.S> Just a pretty hack to move useless lockObject class inside items object (as new Synchronized collection will be created) and make code a little bit clean (at cost of useless lock asquere/releases instructions then you need to perform complex operations with items)

    class Notifier

    {

    public object SyncRoot { get { return items.SyncRoot; }}

    readonly IList items = ArrayList.Synchronized(new ArrayList());

    public void Add(object o) {

    items.Add(o);

    }

    }

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

  12. Drew Marsh says:

    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

  13. Rob M. says:

    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. 😐

  14. orangy says:

    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.

  15. orangy says:

    Also, (from others comments) this line is potential security risk:

    public object SyncRoot { get { return items.SyncRoot; }}

    ArrayList returns ‘this’ as SyncRoot (at least in FW1.1) and thus users, who know this fact, can obtain SyncRoot, cast it to ArrayList and go into your internals directly. What if this is TrustedAppDomains collection? 🙂

    Of course, it would be better if ArrayList is fixed itself, but as we have what we have – don’t expose ArrayList.SyncRoot as own SyncRoot.

  16. orangy says:

    Also, (from others comments) this line is potential security risk:

    public object SyncRoot { get { return items.SyncRoot; }}

    ArrayList returns ‘this’ as SyncRoot (at least in FW1.1) and thus users, who know this fact, can obtain SyncRoot, cast it to ArrayList and go into your internals directly. What if this is TrustedAppDomains collection? 🙂

    Of course, it would be better if ArrayList is fixed itself, but as we have what we have – don’t expose ArrayList.SyncRoot as own SyncRoot.

  17. 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

  18. TAG says:

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

    }

    }

  19. Felipe says:

    We should declare a static object variable, and lock the static object variable, not the this reference.

  20. a. says:

    class Notifier

    {

    ArrayList items = new ArrayList();

    public void Add(object o) {

    lock (items.SyncRoot) {

    items.Add(o);

    }

    }

    }

  21. If all you want is thread-safe access to an instance of ArrayList you can use the ArrayList.Synchronized method which will return a thread-safe instance of ArrayList. Then items.Add would be thread safe.

    I guess the biggest problem with this code is the line

    lock (this) {

    Of course it depends on what you are trying to achieve overall (some times locking on this is necessary), but most typically locking on this is a bad idea. It won’t be a problem if "items" is never exposed outside of "Notifier" and "this" is locked only when accessing "items", but it is generally good practice to be more granular than "lock (this)".

    This is probably being too picky but Add should also return the index of the position at which the item was added.