ConcurrentThrottledList - Will this implementation be thread safe?

Question

I want a thread safe list with a max item count, will the following implementation correctly provide this?

public class ConcurrentThrottledList<T>
{
    private readonly List<T>  items = new List<T>();
    private readonly int maxItems;
    private object _lock = new object();

    public ConcurrentThrottledList(int maxItems)
    {
        this.maxItems = maxItems;
    }

    public bool TryAdd(T item)
    {
        lock (_lock)
        {
            if (items.Count < maxItems)
            {
                items.Add(item);
                return true;
            }
            return false;
        }
    }

    public bool TryRemove(T item)
    {
        lock (_lock)
        {
            return items.Remove(item);
        }
    }

    public bool IsFull { get { lock (_lock) { return items.Count >= maxItems; } } }

    public T[] Items { get { lock(_lock) { return items.ToArray(); } } }
}

Answer 1

@ChrisW made valuable comments, I'll add mine:

Well done:

• I like that private fields are all¹ readonly, this makes intent explicit, which works towards greater maintainability.
• I like that the class is highly cohesive and focused, this works towards greater readability, maintainability and extensibility.

Nitpicks:

• I don't see why _lock gets an underscore and items and maxItems don't; being consistent with the underscore prefix for private fields would mootinate the this qualifier in the constructor.

• Items could be called ToArray, making it more consistent with the framework's lingo, and more representative of its semantics.

• TryAdd and TryRemove both look like they were named after the TryParse pattern; a developper looking at the API would then also be expecting an Add and a Remove method:

  • Add would add the specified item, throw an exception if it can't, and return void.
  • TryAdd would add the specified item, return true if successful and false otherwise.
  • Remove would remove the specified item, throw an exception if it can't, and return void.
  • TryRemove would remove the specified item, return true if successful and false otherwise.

However, List<T>.Remove() is already taking care of returning false if the specified item cannot be removed - throwing an exception in that case would be utter nonsense. Therefore, I'll second @ChrisW in saying that TryRemove should be simply called Remove.

I agree with TryAdd being called as such, because I would seriously expect an Add method to return void, so I'd consider also having an Add method that throws some CapacityExceededException (or whatever is more appropriate) when it can't add the specified item.

---

_{¹ I would be led to believe _lock could also be made readonly, but I don't work with enough multithreaded code to know whether that would/could impact anything.}

Answer 2

will the following implementation correctly provide this?

Yes, it would appear so.

Minor comments:

• TryRemove could be called Remove (because List.Remove is called Remove).
• IsFull may not be useful (because it may be obsoleted by a subsequent Remove by another thread, before this thread is able to act on the IsFull return value)
• Do you want to be able to remove any element? If not, if you only want to remove the first or last element, then ConcurrentQueue or ConcurrentStack might be better than List (in which case only your TryAdd method would need your own explicit lock).

Answer 3

In addition to the existing answers:

1. I concur that IsFull is probably not all that useful.

2. _lock should be readonly

3. Your class is called ConcurrentThrottledList but you don't implement any of the collection interfaces. If you want to make it more useful consider implementing IEnumerable<T>, ICollection<T> and possibly IList<T>.