Monitor.Enter does not block other tasks

Question

I need a synchronization mechanism to allow only one unique item to be proceeded concurrently. So I used Monitor.Enter to block other concurrent execution on the same item.

Here is a cutted version of my code and its unit test to validate the logic.

But I see that some items in my collection can acquire the lock from Monitor.Enter more than one which shouldn't be occured because I don't release the lock when any item is get.

Why I see some of the item in currencies collection has 2 or sometimes 3 as value?

[TestClass]
public class UnitTest2
{
    public static ConcurrentDictionary<string, object> _keyLocks = 
        new ConcurrentDictionary<string, object>();

    public static object AcquireLock(string item)
    {
        object obj = _keyLocks.GetOrAdd(item, new object());
        Monitor.Enter(obj);
    }

    [TestMethod]
    public void AcquireLock_MultipleRequest_OnlyAllow1Request()
    {
        Dictionary<string, int> currencies = new Dictionary<string, int>() {
            { "USD",0 },
            { "EUR",0 },
            { "TRY",0 },
            { "AUD",0 },
            { "PLN",0 }
        };

        int totalTask = 1000;

        List<Task> tasks = new List<Task>();

        for (int i = 0; i < totalTask; i++)
        {
            string curr = currencies.Keys.ElementAt(i % currencies.Count);
            tasks.Add(Task.Factory.StartNew((obj) =>
            {
                string currStr = (string)obj;
                AcquireLock(currStr);
                currencies[currStr] += 1;

                //Monitor.Exit will be implemented 
            }, curr));
        }

        Thread.Sleep(10000);

        foreach (var item in currencies.Keys)
        {
            Assert.AreEqual(1, currencies[item]);
        }
    }
}

You don't need a lock for adding to a `ConcurrentDictionary` - use `GetOrAdd()`. — xxbbcc, Nov 01 '17 at 21:17
Thanks for the comment, i used GetOrAdd first but after thls problem shown i suspect that its causing the problem, because i read that GetOrAdd with overloaded version valuefactory is not thread safe, it can be called multiple times so i thought i was getting different new objects for the same key. — Yucel, Nov 01 '17 at 21:23
There's nothing wrong with the value factory if you know how to use it. Especially in your case, since it's just `new object()` - even if it ends up getting called more than once, only one object is actually placed in the map. It's a lot more efficient than your current code. — xxbbcc, Nov 01 '17 at 21:26
If you have the lock, you don't need a ConcurrentDictionary. — MineR, Nov 01 '17 at 21:53
Possible duplicate of https://stackoverflow.com/questions/4846010/lock-aqcuired-and-further-attempts-to-lock-do-not-block-are-c-sharp-locks-re-en — Peter Duniho, Nov 02 '17 at 00:26
I updated the add logic, remove the lock and change it to use GetOrAdd. — Yucel, Nov 02 '17 at 05:51

score 1 · Accepted Answer · answered Nov 02 '17 at 00:21

1

Why I see some of the item in currencies collection has 2 or sometimes 3 as value?

Because you are using the thread pool to acquire the locks. Using the thread pool means that multiple operations may be executed in the same thread, and of course the monitor for an object is acquired on a per-thread basis. I.e. a given thread can acquire the same lock more than once.

Do also take the comments posted under your question to heart. You are mixing synchronization mechanisms in a confusing, redundant way.

answered Nov 02 '17 at 00:21

Peter Duniho

68,759
7
102
136

so you mean that, one task which is running in ThreadX acquire lock on X object, then after task finish his job ThreadX released to the pool and another task started to run on ThreadX again so that acquire lock on X object can be possible, because ThreadX already acquire the lock with the old task.? Am I right? If yes how I will achieve to block other tasks to acquire lock? – Yucel Nov 02 '17 at 06:04
_"because ThreadX already acquire the lock with the old task.?"_ -- yes, that's right. _"If yes how I will achieve to block other tasks to acquire lock?"_ -- if you read the link I proposed (above, in the comment under your question) as duplicate for your question, you'll see [this answer](https://stackoverflow.com/a/4846160) that describes a variety of options, including synchronization mechanisms that _don't_ have thread affinity. – Peter Duniho Nov 02 '17 at 06:22

Monitor.Enter does not block other tasks

1 Answers1