Concurrent HashSet

Question

I've recently been using HashSet and locking on each method, I found this to not only be a lot of work (was using it in a lot of places) but I started to see inconsistency in my code.

I later decided to make a Concurrent version of HashSet, the reason I don't use a version of ConcurrentDictionary is because I only need a value, but can't duplicate the values so I also can't use a ConcurrentBag, I found HashSet to be my best option, but it lacked a Concurrent thread safe version, this is where my class came in...

I have a few questions about this. Mainly, is this implementation thread-safe and do I have anything to worry about? Is there anything else I can do to make it better, and am I right in using a ConcurrentHashSet, or does anyone have any better ideas for a collection in mind?

One final thing was that I coded the IEnumerator myself whilst half asleep as the original lacked the functionality for this, and I needed to use it inside of foreach loops. I'm pretty sure the rest is thread safe but I guess I'm saying I'm not confident enough with the code I wrote to be thread-safe, its just it seems to step out of the class a bit with a Interface and I was worried that may affect it in some kind of way.

IEnumerator:

public IEnumerator<T> GetEnumerator()
{
    _lock.EnterWriteLock();

    try
    {
        return _hashSet.GetEnumerator();
    }
    finally
    {
        if (_lock.IsWriteLockHeld) _lock.ExitWriteLock();
    }
}

IEnumerator IEnumerable.GetEnumerator()
{
    return GetEnumerator();
}

Full Implementation:

public class ConcurrentHashSet<T> : IDisposable, IEnumerable<T>
{
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
    private readonly HashSet<T> _hashSet = new HashSet<T>();

    public IEnumerator<T> GetEnumerator()
    {
        _lock.EnterWriteLock();

        try
        {
            return _hashSet.GetEnumerator();
        }
        finally
        {
            if (_lock.IsWriteLockHeld) _lock.ExitWriteLock();
        }
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public bool TryAdd(T item)
    {
        _lock.EnterWriteLock();

        try
        {
            return _hashSet.Add(item);
        }
        finally
        {
            if (_lock.IsWriteLockHeld) _lock.ExitWriteLock();
        }
    }

    public void Clear()
    {
        _lock.EnterWriteLock();

        try
        {
            _hashSet.Clear();
        }
        finally
        {
            if (_lock.IsWriteLockHeld) _lock.ExitWriteLock();
        }
    }

    public bool Contains(T item)
    {
        _lock.EnterReadLock();

        try
        {
            return _hashSet.Contains(item);
        }
        finally
        {
            if (_lock.IsReadLockHeld) _lock.ExitReadLock();
        }
    }

    public bool TryRemove(T item)
    {
        _lock.EnterWriteLock();

        try
        {
            return _hashSet.Remove(item);
        }
        finally
        {
            if (_lock.IsWriteLockHeld) _lock.ExitWriteLock();
        }
    }

    public int Count
    {
        get
        {
            _lock.EnterReadLock();

            try
            {
                return _hashSet.Count;
            }
            finally
            {
                if (_lock.IsReadLockHeld) _lock.ExitReadLock();
            }
        }
    }

    public T FirstOrDefault(Func<T, bool> predicate)
    {
        _lock.EnterReadLock();

        try
        {
            return _hashSet.FirstOrDefault(predicate);
        }
        finally
        {
            if (_lock.IsReadLockHeld) _lock.ExitReadLock();
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            _lock?.Dispose();
        }
    }
}

Answer 1

General Stuff

Would be nice to have inline documentation (///) on classes and public members, but everything is pretty simple and understandable. Some might question the name _hashSet, which tells you nothing about it's purpose, but it's private so not a massive concern.

You've correctly identified that you can use a ReaderWriterLock to control access, as HashSet is documented as allowing concurrent reads. The class looks thread safe, with the exception of GetEnumerator().

The implementation of IDisposable looks sound.

if (_lock.IsWriteLockHeld) _lock.ExitWriteLock();

Why are you always checking if you hold the lock? As far as I can tell, this is not helping. If you always expect the thread to hold the write lock, then checking whether it is held will prevent a violent crash when this expectation is violated, potentially hiding an insidious bug. This kind of 'defensive' programming looks robust, but writing robust systems is really hard and unless you have a very clear design goal for particular failures, it is probably better to avoid such defensive coding. A silly example would be if you realised one day that Contains(T) was taking a write lock instead of a read lock (which might not be shown up by testing), and changed it to accordingly, only you forgot to clear the read lock instead of the write lock, and duly never release it. Attempting to release the write lock would have crashed violently and taken you to the bug straight-away, instead of leaving the object perpetually read-locked.

Obligatory comment about people not liking ifs without braces...

GetEnumerator()

You are right to be suspicious of this method! The enumerator supplied by HashSet will cause any writes that happen before it is discharged to break the enumerator: "If changes are made to the collection, such as adding, modifying, or deleting elements, the enumerator is irrecoverably invalidated and its behaviour is undefined.". Presently it will just crash if used after a concurrent write, which is by far the best outcome we could hope for!

The problem is of course that your locking only excludes writes whilst you retrieve the enumerator, but the enumerator will necessarily be enumerated after GetEnumerator() has exited.

How you deal with this is a design decision. You really need to ask yourself why you are implementing IEnumerable<T>, and decide if it is really the right thing to do.

• You could write you own enumerator which holds a read lock... but you will find advice telling you not to do this all over the web, mostly for the simple reason that the caller may not consume your enumerator, may consume it incorrectly/partially/lazily, or may not dispose it (as foreach does), all of which means that the read-lock may be held for a very long time. This would provide a 'snap-shot'

• You could take a copy of the set and return that. This would incur memory overheads, but would be thread-safe, and again provide a snap-shot.

• You could stop implementing IEnumerable, and provide separate methods which allow 'enumeration' operations (like FirstOrDefault, which you already provide)). These 'LINQy' methods can allow controlled processing of the enumeration where you can guarantee the read-lock is held no longer that it must, by providing a simple (restrictive) API for consumers.

A note about ConcurrentDictionary

ConcurrentDictionary has a non-trivial implementation, and exploits the nature of Hash Tables to enable finer concurrency. For a production environment, unless you can demonstrate value in using a custom implementation of a ConcurrentSet like this over one which just wraps ConcurrentDictionary, then I would strongly suggest just wrapping ConcurrentDictionary, which we all hope is thread-safe, and may even be more efficient (if requiring a more space to record non-values)`

ConcurrentDictionary has GetEnumerator(), and it makes only limited guarantees (which it clearly documents), but can run concurrently with writes (because it has access to underlying data-structures). This is not to say that you can do whatever you like with its enumerators, however, as they may hold references to old state, and could represent a memory leak if not discharged. This doesn't guarantee a 'snap-shot', as information from some writes may end up in the enumeration. Again, the question is why you want to take an enumeration, and provide an API to facilitate this.