Is this List property thread-safe?

Question

    private List<T> _T;
    private readonly object _syncLock = new object();

    private List<T> MyT
    {
        get
        {
            lock (_syncLock)
                return _T.ToList<T>();
        }
        set
        {
            lock (_syncLock)
                _T = value;
        }
    }

Exact dupe: http://stackoverflow.com/questions/5874317/thread-safe-listt-property — Callum Rogers, May 03 '11 at 19:52
Under the assumption that you won't be updating the current value based on the current value... then yes, it's somewhat thread safe. However, if you have any code that looks like "myTSObj.MyT = myTSObj.MyT + 1", then no... it's really not. You've opened yourself up to race conditions in this case. — cwharris, May 03 '11 at 19:52
I think it's safe, modifying the original list would modify the copy that ToList() makes, not the original. That would then be assigned as the new member. — James Michael Hare, May 03 '11 at 19:57

score 2 · Answer 1 · answered May 03 '11 at 19:52

2

Yes. You have used a member variable as the lock and made sure it can't be changed. That will work fine.

answered May 03 '11 at 19:52

Nik

7,113
13
51
80

1

And yet, you can still have race conditions. – cwharris May 03 '11 at 19:54
How exactly? It's a list, not an int like in your other comment. – Nik May 03 '11 at 19:55
true, but the idea is still the same. Assume your logic adds an item to the list if the item does not already exist in the list. However, another thread running the same logic does the same thing. It's possible to end up with the same value multiple times. To avoid this, your *logic* should be locking down the object cannot possible avoid all situations like this. – cwharris May 03 '11 at 19:57
That is certainly true; however, I believe that is not a race condition specific to this logic. It is instead specific to the using class. This logic makes the list property thread safe. Race conditions are a problem for the using code. – Nik May 03 '11 at 19:59
True. I agree with that, but the question was "Is this thread safe", and the answer is no. it cannot possibly be thread safe, as he hasn't defined the use case for which it needs to be thread safe. If the use case is "I want to be able to read the whole list, and assign the value to a reference of a new list without confusing other classes", then yes. It's thread safe. Otherwise, it's really not. – cwharris May 03 '11 at 20:02
The question was if the property was thread safe, which it is. The use case is irrelevant to the question of thread safety. It may well not be adequate for the use case, but it will prevent thread access complications. – Nik May 03 '11 at 20:05
wow... total brain fart. You can't actually get to the original list... ignore *Everything* I have said. – cwharris May 03 '11 at 20:11
Sorry for being ignorant, @Nik – cwharris May 03 '11 at 20:13
@Nik: I still agree with @Xixonia's earlier comments. How can the 'property' be thread-safe if it can be used in a way that breaks thread-safety. See my answer. I give an example that breaks thread-safety. – Steven May 03 '11 at 20:55

Steven · Accepted Answer · 2011-05-03T21:02:48.700

2

Nope, it's not thread-safe. Look at the following code:

static MyClass<int> sharedInstance = ...;

// Create a list
var list = new List<int>();

// Share the list
sharedInstance.MyT = list;

// list is now shared, this call is not thread-safe.
list.Add(5);

The problem is that you allow consumers to have a reference to an internal data structure. You can solve this problem as follows:

private List<T> MyT
{
    get
    {
        lock (_syncLock)
            return _T.ToList<T>();
    }
    set
    {
        var copy = value.ToList();

        lock (_syncLock)
            _T = copy;
    }
}

edited May 03 '11 at 21:02

answered May 03 '11 at 20:43

Steven

166,672
24
332
435

Do you need to lock the set? As it's just a reference assignment – EpiX Jan 12 '15 at 05:18
@EpiX: Strictly spoken not. Advantage of having a lock is that it adds a memory barrier. This clears the cache and prevents other threads from getting an old (already cached) reference. So it prevents other threads from getting the old value even after `set` is called. But since `lock` is already used in the `get`, the `lock` in `set` is redundant. However, I do like to use lock in both set and get, because this is in most cases required and makes the code less confusing this that case. If we would remove the `lock` from `set`, we will have to comment explicitly why that `lock` is not required – Steven Jan 12 '15 at 13:39

score 1 · Answer 3 · answered May 03 '11 at 19:58

1

Yes, you seem to be safe. If you look at ToList()'s definition, it's:

public static List<TSource> ToList<TSource>(this IEnumerable<TSource> source)
{
    if (source == null)
    {
        throw Error.ArgumentNull("source");
    }
    return new List<TSource>(source);
}

So essentially you are creating a new list which contains the elements of the old list, all under the lock you provide which gives it thread safety.

Now, the CONTENTS of the list will be the same references in both lists, so it doesn't protect you from altering the original objects STORED in the list, it only protects the list itself.

answered May 03 '11 at 19:58

James Michael Hare

37,767
9
73
83

That's ok. I use it with value types. – Xaqron May 03 '11 at 20:08
You can't conclude the solution to be thread-safe, by just looking at the `ToList()` method. In fact, when supplying an instance to this method, while changing it underneath will break it (look in the List.ctor). See my answer. Here I give an example how to introduce a race condition. – Steven May 03 '11 at 21:00

Is this List property thread-safe?

3 Answers3