C# lock free coding sanity check

Question

UPDATED: now using a read-only collection based on comments below

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

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

Specifically, I know that I could use a lock(_lock) or even an Interlocked.Exchange() but I don't believe that there is a need for it in this case. The volatile keyword should be sufficient (to make sure the value isn't cached), no? Can someone please confirm this, or else let me know what I don't understand about threading :)

Answer 1

It depends on what the intent is. The get/set of the list is atomic (even without volatile) and non-cached (volatile), but callers can mutate the list, which is not guaranteed thread-safe.

There is also a race condition that could lose data:

 obj.Data.Add(value);

Here value could easily be discarded.

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