2
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ then use a Set that is thread safe and has a thread-safe RemoveWhere like ConcurrentBag \$\endgroup\$ – ratchet freak Jul 14 '15 at 14:17
  • 1
    \$\begingroup\$ If you explain some more context there might be a more effective data structure for you to use. \$\endgroup\$ – RobH Jul 14 '15 at 14:18
  • \$\begingroup\$ For locking, you can also consider the ReaderWriterLockSlim class. Some more info here. stackoverflow.com/questions/2116957/readerwriterlock-vs-lock \$\endgroup\$ – hocho Jul 14 '15 at 18:17
  • \$\begingroup\$ The set keeps a list of allowed devices that work as clients to a server with some relevant info about them, we query this list for some processes and we need to keep it update, either removing or adding new devices. The list editing, specially deleting elements from it, it's the least common scenario. The ReadWriterLockSlim does look good though, I'll look into it. But I'm still open to suggestions. \$\endgroup\$ – gonzalo Jul 15 '15 at 9:10
  • 1
    \$\begingroup\$ @gonzalo - so what type is stored in your hashset? Do you always operate with a string and an int for adding/removing? More code would be useful. Your question is close to being off topic as the code looks like a hypothetical/stub example. \$\endgroup\$ – RobH Jul 15 '15 at 12:51
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.