5

Jumping off from this thread, I'm trying to use ConcurrentDictionary to replicate the following:

public static class Tracker
{
    private static Dictionary<string, int> foo = new Dictionary<string, int>();
    private static object myLock = new object();

    public static void Add(string bar)
    {
        lock(myLock)
        {
            if (!foo.ContainsKey(bar))
                foo.Add(bar, 0);

            foo[bar] = foo[bar] + 1;
        }
    }


    public static void Remove(string bar)
    {
        lock(myLock)
        {
            if (foo.ContainsKey(bar))
            {
                if (foo[bar] > 0)
                    foo[bar] = foo[bar] - 1;
            }
        }
    }
}

My initial attempt is:

public static class Tracker2
{
    private static ConcurrentDictionary<string, int> foo = 
        new ConcurrentDictionary<string, int>();

    public static void Add(string bar)
    {
        foo.AddOrUpdate(bar, 1, (key, n) => n + 1);
    }    

    public static void Remove(string bar)
    {
        // Adding a 0'd item may be ok if it wasn't there for some reason,
        //  but it's not identical to the above Remove() implementation.
        foo.AddOrUpdate(bar, 0, (key, n) => (n > 0) ? n - 1 : 0);
    }
}

Is this correct usage? Will I avoid situations such as:

  1. Thread 1: calls Add("a"), foo["a"] is now 1.
  2. Thread 1 is swapped out for Thread 2.
  3. Thread 2: calls Remove("a"), foo["a"] is now 0.
  4. Thread 2 is swapped out for Thread 1.
  5. Thread 1: Requests foo["a"] and assumes the value is 1 but it's actually 0.
Community
  • 1
  • 1
Bullines
  • 5,626
  • 6
  • 53
  • 93

3 Answers3

3

You can't avoid the inconsistencies you are worried about just by using a ConcurrentDictionary. You need something much, much stronger and robust to guarantee that. Ask yourself if you really need that level of consistency before embarking on solving the problem. Dragons be there.

To repeat myself a little differently: ConcurrentDictionary only ensures multiple threads hitting the dictionary won't muck it up. It doesn't guarantee anything about consistency of the values when pulled successively by multiple threads. It can't keep you from shooting yourself in the foot.

jason
  • 236,483
  • 35
  • 423
  • 525
1

The Add method is not equivalent: your original will first add 0 at foo[bar] if nothing exists there, then increment, with a total result of 1 at foo[bar]. The second one will add a 0 if nothing exists there, and will only perform the increment on subsequent calls.

The Remove method is not equivalent: your original will do nothing if foo.ContainsKey(bar) is false, whereas the second one will add a value of 0 for that key.

Did you read the documentation for AddOrUpdate?

EDIT: after you edited to take care of the problems above, the answer to your question is more clearly "no, it will not solve your problems." The reason is the Remove method: its operation is now non-atomic, as there are two separate atomic operations: TryGetValue and foo[bar] = currValue - 1. The other thread could get in between those operations and cause the inconsistencies you are worried about.

The point of methods like AddOrUpdate or GetOrAdd is to make common operations as atomic as possible. Unfortunately it looks like they haven't atomicized your case with ConcurrentDictionary; there is no UpdateIfExists.

However, I am pretty sure that the following will solve your problems: the Interlocked.Decrement method. This solves the following case:

  1. Remove is called on thread 1; foo[bar] has value 2, which gets stored in currValue.
  2. Thread 2 takes over, and Add is called there; foo[bar] gets incremented to 3.
  3. Back to thread 1, which sets foo[bar] to value currValue - 1 = 1.

I tried to think of other cases that might break, but couldn't... which might just mean that I'm not as good as the other commenters who are naysaying, though :P.

EDIT 2: I thought of a problem with using Interlocked.Decrement: it won't only decrement if its value is positive :(.

Domenic
  • 110,262
  • 41
  • 219
  • 271
  • Good point. Edited to reflect equivalence with Dictionary implementation. – Bullines Nov 20 '10 at 19:35
  • What are `s_ConcurrentRequests` and `url`? Also I updated my answer to point out an inequivalence with the `Add` method as well. – Domenic Nov 20 '10 at 19:37
  • Interlocked.Decrement might be ok, but I would have to ensure the values are greater than 0. This might be ok, because the values should never be less than zero. But the trick will be to have a safe way to check that the value is still greater than zero. Perhaps Interlock.Exchange? – Bullines Nov 20 '10 at 19:58
1

Short answer: No, the concurrentdictionary will not protect against the sequence you describe.

But then again, neither will your earlier code.

If you need a guarantee that an object in the collection will stick around for the duration of a series of operations on one thread, assign the object to a local variable in the thread and only use the local variable. Then your thread code doesn't care what happens to the object in the collection.

dthorpe
  • 35,318
  • 5
  • 75
  • 119