Search code examples
c#multithreadinglocking.net-4.0concurrentdictionary

ConcurrentDictionary: TryRemove within a lock?


I believe this works, I've tested it with multiple concurrent threads (though not exhaustively for race conditions and deadlocks):

public static System.Collections.Concurrent.ConcurrentDictionary<string, item> dict =
        new System.Collections.Concurrent.ConcurrentDictionary<string, item>();
public static item dump;

...

foreach (System.Collections.Generic.KeyValuePair<string, item> x in dict)
{
    lock (x.Value)
    {
        if (x.Value.IsCompleted)
        {
            dict.TryRemove(x.Key, out dump);
        }
    }
}

This question is sort of a continuation of this question:

Can I remove items from a ConcurrentDictionary from within an enumeration loop of that dictionary?

And this question:

Updating fields of values in a ConcurrentDictionary

In that I'm doing two "dicey" maneuvers:

  1. Removing values from a ConcurrentDictionary while at the same time enumerating through it (which seems to be ok).
  2. Locking the Value portion of a ConcurrentDictionary. Necessary because manipulating fields of the value is not thread safe, only manipulating the values themselves of the ConcurrentDictionary is thread safe (the code above is a snippet of a larger code block in which fields of values are actually manipulated).

Solution

  • Removing values from a concurrent dictionary while iterating over it is fine. It may have some performance implications (I'm not sure) but it should work.

    Note that you're not locking on something internal to the ConcurrentDictionary - you're locking on the monitor associated with an item object. I personally wouldn't want to do that: either these items should be thread-safe (making it okay to manipulate them anyway) or (preferrably) immutable so you can observe them from any thread without locking. Or you could just make the individual property you're checking thread-safe, of course. Document whatever you do!

    Finally, your use of out dump seems slightly dubious. Is the point just to have something to give TryRemove? If so, I'd use a local variable instead of a static one.