C # lock free coding check

Question

UPDATED : now use the read-only collection based on the comments below

I believe that the following code should be thread safe "lock free", but I want to make sure that I have something missing ...

public class ViewModel : INotifyPropertyChanged { //INotifyPropertyChanged and other boring stuff goes here... private volatile List<string> _data; public IEnumerable<string> Data { get { return _data; } } //this function is called on a timer and runs on a background thread private void RefreshData() { List<string> newData = ACallToAService(); _data = newData.AsReadOnly(); OnPropertyChanged("Data"); // yes, this dispatches the to UI thread } } 

In particular, I know that I could use lock(_lock) or even Interlocked.Exchange() , but I do not believe that this is necessary. The volatile keyword should be sufficient (to make sure the value is not cached), no? Can someone please confirm this, or let me know that I do not understand about streaming :)

Answer 1

It depends on what kind of intention it is. The get / set list is atomic (even without volatility) and not cached (volatile), but callers can mutate the list, which is not guaranteed for threads.

There is also a race condition that may lose data:

  obj.Data.Add(value); 

Here the value can be easily discarded.

I would use an immutable (read-only) collection.

Answer 2

I do not know if it is “safe” or not; it depends on what you mean by "safe." For example, if you define “safe” because “the consistent order of all volatile records is guaranteed to be respected from all threads”, then your program does not guarantee “security” on all hardware.

The best practice here is to use a lock unless you have a very good reason. What is your very good reason to write this risky code?

UPDATE: I want to say that code with low or no lock is extremely risky and that in fact only a small number of people in the world understand this. Let me give you an example from Joe Duffy:

 // deeply broken, do not use! class Singleton { private static object slock = new object(); private static Singleton instance; private static bool initialized; private Singleton() {} public Instance { get { if (!initialized) { lock (slock) { if (!initialized) { instance = new Singleton(); initialized = true; } } } return instance; } } } 

This code does not work; It’s quite correct for the correct implementation of the C # compiler to write you a program that returns null for the instance. Do you see how? If not, then you do not have a business doing programming with low or no lock; you are mistaken.

I cannot figure it out myself; it breaks my brain. This is why I try to never do low-charge programming, which deviates from standard methods that were analyzed by experts.

Answer 3

I think that if you have only two threads, as you described, your code is correct and safe. And also you do not need this variability, it is useless here.

But please do not call it "thread safe", as it is only safe for your two threads, using it in a special way.

Answer 4

I believe that this is safe in itself (even without variability), however, problems may arise depending on how other threads use the Data property.

Provided that you can guarantee that all other threads will read and cache the Data value once before doing an enumeration on it (and do not try to use it for a wider interface to perform other operations), and not allow consistency for second access to the property, then you should be fine. If you cannot make this guarantee (and it would be difficult to make this guarantee if, for example, one of the users is the card itself through data binding and, therefore, a code that you do not control), then you cannot say that it is safe .

For example, this would be safe:

 foreach (var item in x.Data) { // do something with item } 

And that would be safe (provided that the JIT does not allow optimizing the local one, which, in my opinion, is like this):

 var data = x.Data; var item1 = FindItem(data, a); var item2 = FindItem(data, b); DoSomething(item1, item2); 

The above two may work with outdated data, but will always be consistent data. But this will not necessarily be safe:

 var item1 = FindItem(x.Data, a); var item2 = FindItem(x.Data, b); DoSomething(item1, item2); 

It can be a search for two different states of the collection (before and after some thread replaces it), therefore it is unsafe to work with elements that are in each separate enumeration, since they may not correspond to each other.

The problem will be worse with a wider interface; eg. if Data out IList<T> you will need to keep track of the consistency of Count and indexer operations.