C# thread safety with get/set

Question

This is a detail question for C#.

Suppose I've got a class with an object, and that object is protected by a lock:

Object mLock = new Object();
MyObject property;
public MyObject MyProperty {
    get {
         return property;
    }
    set { 
         property = value; 
    }
}

I want a polling thread to be able to query that property. I also want the thread to update properties of that object occasionally, and sometimes the user can update that property, and the user wants to be able to see that property.

Will the following code properly lock the data?

Object mLock = new Object();
MyObject property;
public MyObject MyProperty {
    get {
         lock (mLock){
             return property;
         }
    }
    set { 
         lock (mLock){
              property = value; 
         }
    }
}

By 'properly', what I mean is, if I want to call

MyProperty.Field1 = 2;

or whatever, will the field be locked while I do the update? Is the setting that's done by the equals operator inside the scope of the 'get' function, or will the 'get' function (and hence the lock) finish first, and then the setting, and then 'set' gets called, thus bypassing the lock?

Edit: Since this apparently won't do the trick, what will? Do I need to do something like:

Object mLock = new Object();
MyObject property;
public MyObject MyProperty {
    get {
         MyObject tmp = null;
         lock (mLock){
             tmp = property.Clone();
         }
         return tmp;
    }
    set { 
         lock (mLock){
              property = value; 
         }
    }
}

which more or less just makes sure that I only have access to a copy, meaning that if I were to have two threads call a 'get' at the same time, they would each start with the same value of Field1 (right?). Is there a way to do read and write locking on a property that makes sense? Or should I just constrain myself to locking on sections of functions rather than the data itself?

Just so that this example makes sense: MyObject is a device driver that returns status asynchronously. I send it commands via a serial port, and then the device responds to those commands in its own sweet time. Right now, I have a thread that polls it for its status ("Are you still there? Can you accept commands?"), a thread that waits for responses on the serial port ("Just got status string 2, everything's all good"), and then the UI thread which takes in other commands ("User wants you to do this thing.") and posts the responses from the driver ("I've just done the thing, now update the UI with that"). That's why I want to lock on the object itself, rather than the fields of the object; that would be a huge number of locks, a, and b, not every device of this class has the same behavior, just general behavior, so I'd have to code lots of individual dialogs if I individualized the locks.

Solution 1

No, your code won't lock access to the members of the object returned from MyProperty. It only locks MyProperty itself.

Your example usage is really two operations rolled into one, roughly equivalent to this:

// object is locked and then immediately released in the MyProperty getter
MyObject o = MyProperty;

// this assignment isn't covered by a lock
o.Field1 = 2;

// the MyProperty setter is never even called in this example

In a nutshell - if two threads access MyProperty simultaneously, the getter will briefly block the second thread until it returns the object to the first thread, but it'll then return the object to the second thread as well. Both threads will then have full, unlocked access to the object.

EDIT in response to further details in the question

I'm still not 100% certain what you're trying to achieve, but if you just want atomic access to the object then couldn't you have the calling code lock against the object itself?

// quick and dirty example
// there's almost certainly a better/cleaner way to do this
lock (MyProperty)
{
    // other threads can't lock the object while you're in here
    MyProperty.Field1 = 2;
    // do more stuff if you like, the object is all yours
}
// now the object is up-for-grabs again

Not ideal, but so long as all access to the object is contained in lock (MyProperty) sections then this approach will be thread-safe.

Solution 2

Concurrent programming would be pretty easy if your approach could work. But it doesn't, the iceberg that sinks that Titanic is, for example, the client of your class doing this:

objectRef.MyProperty += 1;

The read-modify-write race is pretty obvious, there are worse ones. There is absolutely nothing you can do to make your property thread-safe, other than making it immutable. It is your client that needs to deal with the headache. Being forced to delegate that kind of responsibility to a programmer that is least likely to get it right is the Achilles-heel of concurrent programming.

Solution 3

As others have pointed out, once you return the object from the getter, you lose control over who accesses the object and when. To do what you're wanting to do, you'll need to put a lock inside the object itself.

Perhaps I don't understand the full picture, but based on your description, it doesn't sound like you'd necessarily need to have a lock for each individual field. If you have a set of fields are simply read and written via the getters and setters, you could probably get away with a single lock for these fields. There is obviously potential that you'll unnecessarily serialize the operation of your threads this way. But again, based on your description, it doesn't sound like you're aggressively accessing the object either.

I would also suggest using an event instead of using a thread to poll the device status. With the polling mechanism, you're going to be hitting the lock each time the thread queries the device. With the event mechanism, once the status changes, the object would notify any listeners. At that point, your 'polling' thread (which would no longer be polling) would wake up and get the new status. This will be much more efficient.

As an example...

public class Status
{
    private int _code;
    private DateTime _lastUpdate;
    private object _sync = new object(); // single lock for both fields

    public int Code
    {
        get { lock (_sync) { return _code; } }
        set
        {
            lock (_sync) {
                _code = value;
            }

            // Notify listeners
            EventHandler handler = Changed;
            if (handler != null) {
                handler(this, null);
            }
        }
    }

    public DateTime LastUpdate
    {
        get { lock (_sync) { return _lastUpdate; } }
        set { lock (_sync) { _lastUpdate = value; } }
    }

    public event EventHandler Changed;
}

Your 'polling' thread would look something like this.

Status status = new Status();
ManualResetEvent changedEvent = new ManualResetEvent(false);
Thread thread = new Thread(
    delegate() {
        status.Changed += delegate { changedEvent.Set(); };
        while (true) {
            changedEvent.WaitOne(Timeout.Infinite);
            int code = status.Code;
            DateTime lastUpdate = status.LastUpdate;
            changedEvent.Reset();
        }
    }
);
thread.Start();

Solution 4

The lock scope in your example is in the incorrect place - it needs to be at the scope of the 'MyObject' class's property rather than it's container.

If the MyObject my object class is simply used to contain data that one thread wants to write to, and another (the UI thread) to read from then you might not need a setter at all and construct it once.

Also consider if placing locks at the property level is the write level of lock granularity; if more than one property might be written to in order to represent the state of a transaction (eg: total orders and total weight) then it might be better to have the lock at the MyObject level (i.e. lock( myObject.SyncRoot ) ... )

Solution 5

In the code example you posted, a get is never preformed.

In a more complicated example:

MyProperty.Field1 = MyProperty.doSomething() + 2;

And of course assuming you did a:

lock (mLock) 
{
    // stuff...
}

In doSomething() then all of the lock calls would not be sufficient to guarantee synchronization over the entire object. As soon as the doSomething() function returns, the lock is lost, then the addition is done, and then the assignment happens, which locks again.

Or, to write it another way you can pretend like the locks are not done amutomatically, and rewrite this more like "machine code" with one operation per line, and it becomes obvious:

lock (mLock) 
{
    val = doSomething()
}
val = val + 2
lock (mLock)
{
    MyProperty.Field1 = val
}

View more solutions