CODEWITHSUNDEEP
c#multithreadinglocking
Ben
Ben

Reputation: 4676

Can I use dictionary elements as lock objects?

I have multiple queues that are being accessed by multiple threads. To achieve thread-safety, I did the following: 

private static Dictionary<string, Queue<string>> MyQueues = new Dictionary<string, Queue<string>>();

public static string GetNextQueueElementForKey(string key)
{
    string res = string.Empty;

    if (MyQueues.Keys.Contains(key))
    { 
       Queue<string> queue = MyQueues[key];
       lock (queue)
       {
           if (queue.Count() > 0)
           {
               res = queue.Dequeue();
           }
       }
   }

   return res;
}

I could also lock MyQueues, but then I would lock more than necessary. So my question is, if locking an object contained in a dictionary is going to work - assuming that a key's value (the queue) is never changed.

Upvotes: 7

Views: 3484

Answers (3)

jason
jason

Reputation: 241789

So my question is, if locking an object contained in a dictionary is going to work - assuming that a key's value (the queue) is never changed.

Looking at your code here:

lock (queue) {
    if (queue.Count() > 0) {
        res = queue.Dequeue();
    }
}

You can, but I would not do this. You should never lock on the object itself, as you might be competing with other threads of code that will lock on the same object, including Queue<T> itself (who could lock on this).

So, at a minimum, you should create a dedicated lock object for each queue.

But, is there a reason that you aren't using ConcurrentQueue<T>? That'd be the simplest solution, and moves the burden of getting it right to the Framework.

Upvotes: 1

Marc Gravell
Marc Gravell

Reputation: 1064184

The fact that it is an element in a dictionary is largely irrelevant - as long as it is a reference type (which Queue<string> is) - then each queue, when fetched from the dictionary, is going to be the same object instance each time. This means it will work perfectly reasonably with locking at the per-queue level.

So basically: yes, that should work fine - as long as the Enqueue does the same per-queue locking. As Jon notes - whether you should do that is another question.

Personally, I'm still of the opinion that Monitor should have been a non-static type, and that you could only lock Monitor instances, rather than any object.

Upvotes: 2

Jon Skeet
Jon Skeet

Reputation: 1503799

You can - but I generally wouldn't. Personally I usually attempt to lock on plain System.Object instances which aren't used for anything else, and ideally aren't even exposed to any code other than the class locking on them. That way you can be absolutely sure that nothing else is going to lock.

In this case it looks like you've got control over the queues so you know they won't be used by other code, but it's possible that the code inside Queue<T> will lock on this. That's probably not the case, but it's the kind of thing I would worry about.

Fundamentally, I wish that .NET hadn't taken Java's approach of "a monitor for every object" - I wish Monitor had been an instantiable class.

(I assume you're only actually reading from the dictionary from multiple threads? It's not safe to use dictionaries for multi-threaded read/write.)

Upvotes: 7

Related Questions