Can I use dictionary elements as lock objects?

Question

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.

Your question reads more like a statement. What is your question again? — Bill Gregg, Jun 07 '13 at 12:51
You don't lock objects, you lock *references to objects*. So in other words, whether the object's value changes does not influence the lock. — Nolonar, Jun 07 '13 at 12:55
@Nolonar no, you lock on the object itself. You could have 17 references to the same object - they would all lock the same thing. — Marc Gravell, Jun 07 '13 at 12:56
@Yogee I don't see what relevance the `GetHashCode()` has here, or why anyone would want to take a `ToString()` on a hashcode - and indeed you should almost certainly *never* lock on a `string` - can you clarify what you are saying there? — Marc Gravell, Jun 07 '13 at 12:58
@MarcGravell Actually, when I said "reference", I meant "address where the object is stored". My bad, sorry. — Nolonar, Jun 07 '13 at 12:58
@MarcGravell: I remove my comment. I check and you are correct. It doesn't lock the hashcode, it lock address of the object. — Yogee, Jun 07 '13 at 13:00
@yogee: It does not lock the *address* of the object; the object can have its address moved by the garbage collector! It locks the synchronization block that is in the referred-to object. Of course, this is an implementation detail. — Eric Lippert, Jun 07 '13 at 13:23

score 7 · Accepted Answer · answered Jun 07 '13 at 12:54

7

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.)

answered Jun 07 '13 at 12:54

Jon Skeet

1,421,763
867
9,128
9,194

I only write to the queue when it is empty. I omitted that part in my code above for simplicity. But the queue should be locked in that case, so writing to it should pose no problems. – Ben Jun 07 '13 at 12:58
1

But using a plain `System.Object` doesn't work for an unknown number of lock objects. – Ben Jun 07 '13 at 13:00
@Ben: You can use a dictionary of dedicated lock objects, one for each instance of `Queue`. I still don't see why you're using `Queue` instead of [`ConcurrentQueue`](http://stackoverflow.com/a/16984743/45914). – jason Jun 07 '13 at 13:00
1

@Ben: You could create a `Dictionary>` - any time you're creating a queue, you create an object. I'm not sure whether I'd do that (and using `ConcurrentQueue` would be my preferred option usually) but it's worth considering. – Jon Skeet Jun 07 '13 at 13:06

score 2 · Answer 2 · answered Jun 07 '13 at 12:55

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.

score 1 · Answer 3 · edited May 23 '17 at 12:34

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.

Can I use dictionary elements as lock objects?

3 Answers3