Lock in properties, good approach?

Question

Hi, In my multithreading application I am using some variables that can be altered by many instances in the same time. It is weird but it has worked fine without any problem..but of course I need to make it thread-safe. I am just beginning with locks so I would appretiate your advice:

When client connects, class Client is created where each Client has its own "A" variable.

Sometimes, Client calls method like that:

Client selectedClient SelectOtherClientClassByID(sentID);

selectedClient.A=5;

No problems until now with that even when 5 classes were doing at the same time (threadpool), but I was thinking what about adding locks to A properties?

Like: A{ get{ return mA; } set{ use lock here for settting A to some value

} }

Would it be OK?

Answer 1

You need to use locks in BOTH get and set. This lock must be the same object. For example:

private object mylock = new object();

public int A {

  get {
    int result;
    lock(mylock) {
    result = mA; 
    }
    return result;
  } 

  set { 
     lock(mylock) { 
        mA = value; 
     }
  }
}

Answer 2

It's very rare when all you need is just set a single property. More often selectedClient.A = 5 will be a part of a much bigger logical operation, which involves several assignments/evaluations/etc. During that whole operation you'd rather prefer selectedClient to be in a consistent state and not to introduce deadlocks/race conditions. Therefore, it will be much better to expose SyncRoot property in your Client class and lock on that from the calling code:

Client selectedClient = GetClient(...);

lock(selectedClient.SyncRoot)
{
    selectedClient.A = 5;
    selectedClient.B = selectedClient.A * 54;
}

Answer 3

Locking access to properties inside of accessors may lead to bogus results. For the example, look at the following code:

class C {
    private object mylock = new object();

    public int A {

      get {
        int result;
        lock(mylock) {
        result = mA; 
        }
        return result;
      } 

      set { 
         lock(mylock) { 
            mA = value; 
         }
      }
    }
}
C obj = new C;
C.A++;

(yes, I've copied it from the first answer) There is a race condition here! Operation "C.A++" actually requires two separate accesses to A, one to get the value and the other to set the updated value. Nothing ensures that these two accesses will be carried out as together without context switch between them. Classical scenario for race condition!

So, beware! It's not a good idea to put locks inside accessors, locks should be explicitly obtained, like the previous answer suggests (though it doesn't have to be with SyncRoots, any object will do)