Thread-safe removewhere

Question

I'm trying to implement a HashSet that it's accessed from different thread for querying, adding or deleting elements. It doesn't make sense to use a Dictionary for I got several search keys so I won't get benefited much for the speed provided on querying.

My real problem is when trying to remove several items matching a given definition without locking the List for to long, I use this code right now

   public static void DeleteFromHashSet(string companyId, int deviceId)
   {
        lock (setLock)
        {
            set.RemoveWhere(x => x.CompanyId == companyId && x.y == deviceId);
        }
    }

but this has the problem of locking the access for querying the hashset for the duration of the RemoveWhere, I can change that with a foreach and just lock for each individual item but I wanted to know if there's a better way.

Update:

This is what I finally did, in case it's of some help

    class DeviceLocator
{
    #region  fields

    private HashSet<DeviceLocator> m_deviceList = new HashSet<DeviceLocator>();
    private object setLock = new object();
    private ReaderWriterLockSlim cacheLock = new ReaderWriterLockSlim();

    public  void AddLocator(DeviceLocator locator)
    {
        DeleteLocator(locator);

        cacheLock.EnterWriteLock();

        m_deviceList.Add(locator);

        cacheLock.ExitWriteLock();
    }

    public void AddLocators(IEnumerable<DeviceLocator> locators)
    {
        foreach (DeviceLocator locator in locators)
        {
            AddLocator(locator);
        }
    }

    #endregion Add items

    #region Query items

    public DeviceLocator GetLocator(string serialNumber)
    {
        try
        {
            return ReadSafely(x => x.SerialNumber == serialNumber).Single();
        }
        catch
        {
            throw new LocatorNotFoundException(ExceptionTexts.LocatorNotFoundException, null, ImportanceLevel.Low, m_ID);
        }
    }

    public IEnumerable<DeviceLocator> GetLocatorsFromCompany(string companyId)
    {
        return ReadSafely(x => x.CompanyID == companyId);
    }

    public IEnumerable<string> GetLocatorsStringFromCompany(string companyId)
    {
        return GetLocatorsFromCompany(companyId).Select(y => y.SerialNumber);
    }

    public IEnumerable<DeviceLocator> GetsLocatorBasedOnConnectionType(ConnectionType type)
    {
        return ReadSafely(x => x.ConnectionType == type); ;
    }

    private IEnumerable<DeviceLocator> ReadSafely(Func<DeviceLocator, bool> predicate)
    {
        try
        {
            cacheLock.EnterReadLock();

            var result = m_deviceList.Where(predicate);

            return result;
        }
        catch
        {
            throw;
        }
        finally
        {
            cacheLock.ExitReadLock();
        }
    }

    public  void DeleteLocator(DeviceLocator locator)
    {
        DeleteSafely(x => x.CompanyID == locator.CompanyID && x.SerialNumber == locator.SerialNumber);
    }

    /// <summary>
    /// Removes all locators from given company
    /// </summary>
    /// <param name="companyId"></param>
    public  void DeleteLocator(string serialNumber)
    {
        DeleteSafely(x => x.SerialNumber == serialNumber);
    }

    public  void DeleteLocatorsFromCompany(string companyId)
    {
        DeleteSafely(x => x.CompanyID == companyId);
    }

    private  void DeleteSafely(Predicate<DeviceLocator> predicate)
    {
        try
        {
            cacheLock.EnterWriteLock();

            m_deviceList.RemoveWhere(predicate);
        }
        catch
        {
            throw;
        }
        finally
        {
            cacheLock.ExitWriteLock();
        }
    }
}

Answer 1

Firstly, the biggest problem is that ReaderWriterLockSlim implements IDisposable so your DeviceLocator class should too as it holds a reference at the class level.

---

You could make better use of the locks if you moved things round a bit:

public void AddLocators(IEnumerable<DeviceLocator> locators)
{
    cacheLock.EnterWriteLock();
    foreach (DeviceLocator locator in locators)
    {
        AddLocatorUnsafe(locator);
    }
    cacheLock.ExitWriteLock();
}

private void AddLocatorUnsafe(DeviceLocator locator)
{
    m_deviceList.RemoveWhere(x => x.CompanyID == locator.CompanyID 
            && x.SerialNumber == locator.SerialNumber);

    m_deviceList.Add(locator);
}

Now you only acquire the lock once and do everything while holding that lock instead of having to keep acquiring and releasing between items.

---

Don't do this:

try
{
    // ...
}
catch
{
   throw;
}
finally
{
    // ...
}

That catch block is entirely redundant - remove it.

---

Some people would probably tell you off for using regions. I'm not really that fussed about it but you should be aware that a lot of C# developers don't like using regions for splitting up classes.