3

I have been reading around and am getting conflicting answers on whether I should or should not use synclock on properties.

I have a multi-threaded application that needs to get/set properties across threads on instance objects. It is currently implemented without using synclock and I have not noticed any problems so far. I am using synclock on common static methods but I'd like to implement my instance classes properly and in a thread safe way.

Any feedback would be greatly appreciated.

Netpuppy
  • 33
  • 1
  • 3
  • Would you need to lock ("SyncLock") if you were using a method, say, `SetX()` instead of the `X` property setter? Without a full context, it's hard to say. Also, without knowing the larger context, it's not possible to discern if individual locks solve/address the *real* issues. "Threading is easy. Concurrency is hard." –  Nov 25 '10 at 00:35
  • Perhaps see: http://stackoverflow.com/questions/505515/c-thread-safety-with-get-set –  Nov 25 '10 at 00:36
  • @pst I have read a few things on replacing properties with methods and cannot seem to get a clear answer on that either. As I understand it, a property is essentially a simple method. Since I have implemented the application using the property/field design I would like to try and keep it that way. I guess the question is more about good design practises and thread-safety. Thanks for the link, I thought I'd done a thorough search but never saw that one! – Netpuppy Nov 25 '10 at 00:43
  • The real point of replacing properties with methods is "Tell, don't ask." If you need to do something with the data in a class, move that code into the class as a method, instead of putting it in a form or module and having it use properties to get at the data. See http://c2.com/cgi/wiki?TellDontAsk for a discussion. – Jeffrey Hantin Nov 25 '10 at 00:46
  • @Jeffery hmmm... that is an interesting article. My application depends on state behavior so that throws a cat amongst the pigeons. – Netpuppy Nov 25 '10 at 01:00

2 Answers2

5

A good rule of thumb is that you need to lock if any of the following conditions hold true:

  • if any field of an object is going to be modified on more than one thread
  • if any modifications involve accessing more than one field
  • if any modifiable field is a Double, Decimal, or structured value type
  • if any modifications involve read-modify-write (i.e. adding to a field or setting one field with the value from another)

then you probably need to lock in every method or property that accesses those fields.

EDIT: Keep in mind that locking inside of a class is rarely sufficient -- what you need to do is make sure that things don't go wrong across the span of an entire logical operation.

As @Bevan points out, if calling code needs to access an object more than once, the client code should take out its own lock on the object for the entire duration of its work to ensure that another thread doesn't get "in between" its accesses and foul up its logic.

You also need to take care that if anything needs to take out multiple locks at once, that they always be taken in the same order. If thread 1 has a lock on instance A and tries to lock instance B, and thread 2 has a lock on instance B and tries to get a lock on instance A, both threads are stuck and unable to proceed -- you have a deadlock.

Jeffrey Hantin
  • 35,734
  • 7
  • 75
  • 94
  • Thanks, that is very useful information and at least 2 of those conditions ARE being met so locking sounds like the way to go. The fields are always private but some methods in the same class are modifying the fields. All others must use the public properties as they also fire events when changes occur. – Netpuppy Nov 25 '10 at 00:48
  • That makes things a lot clearer, thanks for your help and the quick responses. You guys are awesome! – Netpuppy Nov 25 '10 at 01:21
2

You can't make an object thread safe just by surrounding individual methods with locks. All you end up doing is serialising (slowing down) access to the object.

Consider this minor example:

var myObject = ...
var myThreadSafeList = ...
if (!myThreadSafeList.Contains(myObject))
{
    myThreadSafeList.Add(myObject);
}

Even if myThreadSafeList has every method locked, this isn't threadsafe because another thread can alter the contents of the list between the calls to Contains() and Add().

In the case of this list, an additional method is needed: AddIfMissing():

var myObject = ...
var myThreadSafeList = ...
myThreadSafeList.AddIfMissing(myObject);

Only by moving the logic into the object can you surround both operations with the lock and make it safe.

Without further details, it's hard to comment futher, but I'd suggest the following:

  • Make all properties read-only, and allow anyone to read them at any time
  • Provide mutator methods that take sets of properties that get modified together, and make the changes atomically within a lock

To illustrate:

public class Person {
    public string FullName { get; private set; }
    public string FamilyName { get; private set; }
    public string KnownAs { get; private set; }

    public void SetNames( string full, string family, string known) {
        lock (padLock) {
            ...
        }
    }
}
Bevan
  • 43,618
  • 10
  • 81
  • 133
  • Nice... so essentially you are saying it is no problem to get from the property but anytime I need to set it I should do so using a call to a method that locks the 'fields' being modified and therefore bypassing the property set action? – Netpuppy Nov 25 '10 at 01:07
  • You want to make sure that any operation that changes your object is "atomic" and won't be interrupted by a context change in mid-execution. You do this by locking the object, not the fields. Whether you use private property setters or directly access a backing field is up to you - it doesn't affect things. – Bevan Nov 25 '10 at 08:23