Is returning an IEnumerable<> thread-safe?

Question

I have a Visual Studio 2008 C# .NET 3.5 project where I want to have a thread-safe pool of Foo objects.

public class FooPool
{
    private object pool_lock_ = new object();
    private Dictionary<int, Foo> foo_pool_ = new Dictionary<int, Foo>();

    // ...

    public void Add(Foo f)
    {
        lock (pool_lock_)
        {
            foo_pool_.Add(SomeFooDescriminator, f);
        }
    }

    public Foo this[string key]
    {
        get { return foo_pool_[key]; }
        set { lock (pool_lock_) { foo_pool_[key] = value; } }
    }

    public IEnumerable<Foo> Foos
    {
        get
        {
            lock (pool_lock_)
            {
                // is this thread-safe?
                return foo_pool_.Select(x => x.Value);
            }
        }
    }
}

Is the public IEnumerable<Foo> Foos { get; } function thread-safe? Or, do I need to clone the result and return a new list?

Protractor

btw ` get { return foo_pool_[key]; }` is broken. You forgot the lock. – Officer

Because Dictionary, like most .net classes isn't threadsafe. In particular it doesn't allow somebody to read and write at the same time. – Officer

Yes, but the worst case scenario I can think of is the one Eric Lippert outlined below for the Foos function. Which is already an inherent weakness of this code. Is there a scenario where you get something worse than stale data? (i.e one thread tries to get; while another does a set;, the getter may get old data because the set; thread hasn't finished yet.) – Protractor

@PaulH: You can get wrong results if the writer moves stuff around in the dictionary while the read is being done. You need a lock around the read, because it is not atomic. – Gerenuk

Answer 1

No, it isn't.

If another thread adds to the dictionary while your caller enumerates that, you'll get an error.

Instead, you can do:

lock (pool_lock_) {
    return foo_pool.Values.ToList();
}

Topsyturvydom

Answer 2

Is the IEnumerable<Foo> Foos { get; } function thread-safe?

No.

Or, do I need to clone the result and return a new list?

No, because that's not right either. A threadsafe method that gives the wrong answer is not very useful.

If you lock and make a copy then the thing you are returning is a snapshot of the past. The collection could be changed to be completely different the moment the lock is released. If you make this threadsafe by making a copy then you are now handing a bag full of lies to your caller.

When you are dealing with single-threaded code, a reasonable model is that everything is staying the same unless you take specific measures to change a thing. That is not a reasonable model in multi-threaded code. In multi-threaded code, you should assume the opposite: everything is constantly changing unless you take specific measures (such as a lock) to ensure that things are not changing. What is the good of handing out a sequence of Foos that describe the state of the world in the distant past, hundreds of nanoseconds ago? The entire world could be different in that amount of time.

Crankpin

So... Are you suggesting the lock does not go in the pool itself, but should go around any code that uses the pool and that my attempt to make the pool itself threadsafe is misguided? – Protractor

@PaulH: I'm suggesting that you need to think about two questions: (1) what services do the users of this pool actually require the pool to provide? and (2) what services can the pool actually provide, given its multithreaded nature? A multithreaded pool does not typically provide a "list everything in the pool" service because (1) no one needs it, and (2) it's hard to provide that service in a way that is safe, accurate and efficient. A pool typically provides the services "take something out of the pool" and "put something back in the pool". – Crankpin

I appreciate the advice and I will give it a bit more consideration. – Protractor

Answer 3

It is not thread safe. You need to return ToList():

return foo_pool_.Select(x => x.Value).ToList();

Careful of deferred execution!

The fact is the actual code runs after the lock has exited.

// Don't do this
lock (pool_lock_)
{
    return foo_pool_.Select(x => x.Value); // This only prepares the statement, does not run it
}

Yerxa

Answer 4

You may want to consider a SynchronizedCollection,

SynchronizedCollection Class Provides a thread-safe collection that contains objects of a type specified by the generic parameter as elements.

http://msdn.microsoft.com/en-us/library/ms668265.aspx

Boondoggle

Answer 5

If you'll lock on every read access you'll end with very bad performance. And in suggestions to use toList you'll also allocate memory every time.

If you using .NET 4 just use ConcurrentDictionary class from new thread safe collections. They will provide very fast (lock free) mechanisms for accessing data from multiple threads.

http://msdn.microsoft.com/en-us/library/dd997305.aspx

If you are using old .NET version I would suggest you to use for cycle with count variable instead of foreach it will work if you only add elements without removing them (as in your example)

Weatherley