Lock in properties, good approach?

Question

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?

score 19 · Accepted Answer · answered Oct 23 '09 at 09:50

19

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

answered Oct 23 '09 at 09:50

NT_

2,660
23
25

1

Thank you...btw for reading its because it could changed during the reading by other class? – Petr Oct 23 '09 at 09:53
2

It is not a good idea to lock on this, as it is accessible from outside of the type. – Brian Rasmussen Oct 23 '09 at 09:54
It could change while you're setting. The lock acts as a barrier, preventing any other action until the lock owner releases the lock. – NT_ Oct 23 '09 at 09:54
@Brian, I agree. Made the change on my code. Was simply demonstrating. – NT_ Oct 23 '09 at 09:56
Sorry I was on the meeting, _NT please, how did your code look before you change, I mean what was the problem Brian mentioned? I like to learn as much as I can :) – Petr Oct 23 '09 at 10:48
I did lock(this) to lock the object I'm currently on. – NT_ Oct 23 '09 at 11:26

score 6 · Answer 2 · answered Oct 23 '09 at 10:17

6

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)

answered Oct 23 '09 at 10:17

ilial

61
1
2

Also you mean manually add lock object to all calls in the code? I guess there are at least 20 readings and 15 writings iny my code so far. – Petr Oct 23 '09 at 10:49
To do that you expand your lock to cover the entire critical region. You don't do A++. – NT_ Oct 23 '09 at 11:29
NT: I dont understand you well probably. I have around 15 methods with different operations with that variable on different instance. Im not sure how you meant expanding the lock. So the properties lock is not good if I read correctly all above – Petr Oct 23 '09 at 12:07
@Petr: I directed my previous comment to ilial. As for your comment, you would need to establish whether these have a common synchonisation object. In that case you should use a single lock. If you're new to all this have a look at this interesting link: http://www.codeproject.com/KB/threads/ThreadingDotNet3.aspx#CriticalSections – NT_ Oct 23 '09 at 14:31

score 2 · Answer 3 · answered Oct 23 '09 at 09:53

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

answered Oct 23 '09 at 09:53

Anton Gogolev

113,561
39
200
288

I have no other variables sharing than one list and those int. Mainly, I know nothing about SyncRoot yet :) – Petr Oct 23 '09 at 09:58
SyncRoot is not recommended: See http://blogs.msdn.com/brada/archive/2003/09/28/50391.aspx – NT_ Oct 23 '09 at 10:01
@_NT: Quoting from the blog post: "Rest assured we will not make the same mistake as we build the generic versions of these collections." And here we are, 6 years later, with List.SyncRoot without any discouraging notes in the MSDN: http://msdn.microsoft.com/en-us/library/bb356596.aspx – Anton Gogolev Oct 23 '09 at 10:08
MSDN is hardly the best resource for correct programming! SyncRoot, is a public lock and a disaster waiting to happen (according to CLR via C# by Jeffrey Richter http://www.amazon.co.uk/gp/reader/0735621632/ref=sib_dp_pt#reader-link) – NT_ Oct 23 '09 at 10:20

Lock in properties, good approach?

3 Answers3

Linked