I am a threading noob and I am trying to write a custom thread safe generic list class in C# (.NET 3.5 SP1). I've read Why are thread safe collections so hard?. After reviewing the requirements of the class I think I only need to safely add to the list and return the list. The example shows pretty much all I want except it lacks the return list method therefore I've written my own public method like below:
Update: based on suggestions given I've reviewed my requirements and therefore simplified the class to as below:
public sealed class ThreadSafeList<T>
{
private readonly IList<T> list = new List<T>();
private readonly object lockable = new object();
public void Add(T t)
{
lock (lockable)
{
list.Add(t);
}
}
public IList<T> GetSnapshot()
{
IList<T> result;
lock (lockable)
{
result = new List<T>(list);
}
return result;
}
}
Agree with @jrista. There's a semantics issue you need to resolve, and why is it called Translate()
? What is the intent?
A - current code - return a read-only wrapper of the internal list
return new ReadOnlyCollection<T>(list);
You still have threading issues if the original list is changed if another thread is iterating over the list. As long as you're aware of this, it isn't a huge issue.
B - a read-only copy.
return new List<T>(list).AsReadOnly();
This list has no threading issues because nothing modifies the new list. The only reference held is by the ReadOnlyCollection<T>
wrapper.
C - a normal (writable) copy
return new List<T>(list);
Returns a new list, and the caller can do what they wish to their list without affecting the original list, and changes to the original list do not affect this list.
Does it matter if another consumer grabs a copy of the list and then modifies their copy? Do consumers need to see changes to the list? Do you just need a thread-safe enumerator?
public IEnumerator<T> ThreadSafeEnumerator()
{
List<T> copy;
lock(lockable)
copy = new List<T>(list);
foreach (var value in copy)
yield return value;
}
In my experience you have to use your brain when it comes to thread-safety and not rely on solutions such as these.
in short, it depends on what the receiver of the list is going to be doing with it.
The Translate() method looks correct. Using the lock you are preventing others from adding or otherwise modifying your list while you are in Translate/AddRange.
I think there might be a problem with your IsReadyOnly property though. You use a lock when reading/writing the property internally. But there also is a public getter which is not locked. It might happen that thread 1 calls MarkAsReadOnly while a second thread might still get false when looking at IsReadOnly. I'd use a normal property instead and either lock in the getter or use a volatile bool field.
You can use SynchronizedCollection.
http://msdn.microsoft.com/en-us/library/ms668265.aspx