Thread safe access of List properties

Question

I was wondering if this statement would cause sync issues:

List<Character> characters = World.CharacterManager.Characters;

'Characters' is a class

'CharacterManager.Characters' would look something like this:

public List<Character> Characters
{
    get
    {
        lock (this.objLock) { return this.characters; }
    }
}

would this cause synchronization problems?

I want to use the referenced List to iterate through to find the character I am looking for.

We need more context, do you need to be locking at all? Are you using multiple threads? That lock you have doesn't really achieve anything. — Gary, Jan 03 '12 at 22:39
I think multithreading can be safely assumed, as he wouldn't be asking about synchronization in a single-threaded context... — Phil, Jan 03 '12 at 22:50
Thank you Phil. Yes I can confirm that this situation is for a multithreaded application. — TheAJ, Jan 03 '12 at 23:30
[What is this thing you call thread safe?](http://blogs.msdn.com/b/ericlippert/archive/2009/10/19/what-is-this-thing-you-call-thread-safe.aspx). If you don't define what operations you're actually performing, and how you expect it to behave, then we can't know if it will meet your expectations. — Servy, Feb 28 '13 at 21:41
@Servy, look at my question properly, I did explain what I was using it for... besides, this question was answered long time ago. — TheAJ, Mar 06 '13 at 22:47

score 9 · Answer 1 · answered Jan 03 '12 at 22:44

The problem is that you are locking during the get, but once each thread has a reference to the collection, they can both act against it at the same time. Since the List<T>'s members aren't thread safe, you will encounter random bugs and exceptions when iterating, adding, removing, etc the collection.

You probably need to return a thread-safe collection. There isn't a 100% compatible thread-safe version, so you need to look through System.Collections.Concurrent and find a version that you can use.

Andreas · Accepted Answer · 2012-01-04T11:03:37.727

2

The lock is useless that way. You will have to use a thread-safe collection such as Will suggested, or if you don't need write access you can expose only a read only version of your list like so:

public ReadOnlyCollection<Character> Characters {
  get {
    lock (locker) { return this.characters.AsReadOnly(); }
  }
}

These collections cannot be modified, so if your Character type is immutable, you don't have any synchronization issues. If Character is mutable, you have again a problem, but you would have that problem even with a thread-safe collection. I hope you're aware of that. You can also expose the property returning an IList<Character>, but usually I find it better to tell the caller that the object is read only.

If you need write access, you could also do that by providing the appropriate methods at the scope of the CharacterManager and synchronize them. Jesse has written a nice example on how to do this.

EDIT: SyncRoot is not present on ICollection<T>.

edited Jan 04 '12 at 11:03

answered Jan 03 '12 at 23:05

Andreas

6,447
2
34
46

Note that List only implements `SyncRoot` explicitly, so it'd have to be cast to `ICollection` to be accessed. (ref: http://stackoverflow.com/a/4067371/3312) – Jesse C. Slicer Jan 03 '12 at 23:13
Thanks! I also searched and found that they [discontinued this pattern](http://blogs.msdn.com/b/bclteam/archive/2005/03/15/396399.aspx) (a long while ago). I edited the answer. – Andreas Jan 03 '12 at 23:16
Thanks man. If i called something like: World.CharacterManager.Characters.Count would it still be a useless lock? – TheAJ Jan 03 '12 at 23:36
1

The lock that you have stated: yes! The compiler will probably even [remove that lock](http://stackoverflow.com/questions/1359184/can-i-put-a-return-statement-inside-a-lock) as it becomes empty after moving the return statement outside. In the case of returning the result of `AsReadOnly()` the lock is however meaningful as there may be complex operations occurring in that method. – Andreas Jan 04 '12 at 11:02

Jesse C. Slicer · Answer 3 · 2013-02-28T21:39:23.653

Does the calling code actually need to be able to add and remove from the list? If so, that's not considered a best practice. Here's a (possible) way to implement without that requirement, instead putting the adding and removing of Character items into the CharacterManager class itself:

internal sealed class CharacterManager
{
    private readonly IList<Character> characters = new List<Character>();

    public ReadOnlyCollection<Character> Characters
    {
        get
        {
            lock (this.characters)
            {
                return this.characters.AsReadOnly();
            }
        }
    }

    public void Add(Character character)
    {
        lock (this.characters)
        {
            this.characters.Add(character);
        }
    }

    public void Remove(Character character)
    {
        lock (this.characters)
        {
            this.characters.Remove(character);
        }
    }
}

You get a +1 for mentioning mutability issues of the underlying `Character` class. — Jesse C. Slicer, Jan 03 '12 at 23:07

score 0 · Answer 4 · answered Jan 03 '12 at 22:57

If you are only wanting the caller to be able to enumerate the list then your property should have the type IEnumerable instead. If this is the case, then I would also make a copy of that list and return the copy. If the list is changed while you are enumerating it, then it will become invalid and throw an exception. The trade off is that the caller could possibly not have the latest version of the list. I would be inclined to convert it to a method named GetCharactersAsOfNow() instead of a property to help show that the caller needs to get an updated list for each call.

However if you are planning on allowing the caller to modify the list, then you have to make note that the list is not thread safe and require your caller to perform thread synchronization. Given that the caller now has this responsibility then you no longer need the lock in your property getter.

Thread safe access of List properties

4 Answers4