Is this code thread-safe?

Question

I've got a class with several properties. On every value update, a Store method is called which stores all fields (in a file).

private int _Prop1;
public int Prop1 {
    get {
        return _Prop1;
    }
    set {
        _Prop1 = value;
        Store();
    }
}

// more similar properties here...

private XmlSerializer _Ser = new ...;
private void Store()
{
    lock (_Ser) {
        using (FileStream fs = new ...) {
            _Ser.Serialize (fs, this);
        }
    }
}

Is this design thread-safe?

(Btw, if you can think of a more appropriate caption, feel free to edit.)

I think it is thread safe. If properties are changed on multiple threads, the values will be set in random order, atomic Stores will happen in a random order, but in the end, every property will have its latest value, and at the very end, an atomic Store happens, ensuring that the file is up-to-date.

Clarification: The properties will not be set very often, but they may be set at the same time. What matters is having a valid file most of the time.

If a thread is going to change a property with regards to a property value, it has to lock on the whole object to sync with other threads. That's basically the same as with locking on a List on enumeration and is not a responsibility of this class.

Answer 1

It depends what you're calling on different threads.

If you set properties on different threads at once, it isn't thread-safe, because the properties can change while they're being serialized.

Answer 2

I would suggest using some sort of Mutex to ensure the data is only serialized at a particular point, it could do it just as the update changes or is about to change.

The following link takes you to a simple example on the MSDN website that should hopefully demonstrate it for you:

Mutex Example

Answer 3

No.

In the case you want to synchronize the properties theirself, this code is not thread safe, since the 'lock' is not on the _Prop1 value, but only on _Ser. Indeed when a thread is gettin the property, the property could be set by another thread.

Even the serialization process, _Ser access to properties which can be changed by other threads in execution (while _Ser is running, another thread set Prop1).

This code actually disallow the use of the XmlSerialize _Ser object by multiple threads. If it is what you want to get...

---

The answer depends essentially on what you want to get.

Answer 4

If the Prop1 property can be called from multiple threads, make the _Prop1 field volatile.

private volatile int _Prop1;

From MSDN,

The volatile keyword indicates that a field might be modified by multiple threads that are executing at the same time. Fields that are declared volatile are not subject to compiler optimizations that assume access by a single thread. This ensures that the most up-to-date value is present in the field at all times.

Consider it good practice. It's not going to make the code you posted thread safe because, as others have said, while the class is being serialized, the value of _Prop1 can be changed by another thread. Still, if you have a field that is capable of being read and written by multiple threads, marking the field as volatile guarantees that your code sees the most current value.

Answer 5

There's not enough code to make the call. But sure, nothing good is going to happen if you don't serialize write access to the file. The 2nd thread that assigns the property is going to bomb on an IOException if the 1st thread is still busy writing the file.

Fine-grained locking like this is usually a problem. The client code could be busy changing more than one property of the class. You'll get a partial update if an exception is raised, producing a file that contains serialized state that is invalid and may cause trouble when it is read. You'll need something like a BeginUpdate(), EndUpdate() pair.

Answer 6

Not thread safe, unless the property type is atomic.

Simple example, with threads A and B.

A: Prop1 = foo
A: Store()
A:    ... store saves foo.Var1
B: Prop1 = bar
A:    ... store saves bar.Var2 (instead of foo.Var2)

The integrity of the data is possibly compromised. Even with types as simple as Int64, problems can occur in theory.

Putting another lock(_Ser) within the setter would help.