CODEWITHSUNDEEP
c#multithreadingthread-safety
Archeg
Archeg

Reputation: 8462

Multiple locking task (threading)

I need to implement the class that should perform locking mechanism in our framework. We have several threads and they are numbered 0,1,2,3.... We have a static class called ResourceHandler, that should lock these threads on given objects. The requirement is that n Lock() invokes should be realeased by m Release() invokes, where n = [0..] and m = [0..]. So no matter how many locks was performed on single object, only one Release() call is enough to unlock all. Even further if o object is not locked, Release() call should perform nothing. Also we need to know what objects are locked on what threads.

I have this implementation:

public class ResourceHandler
{
    private readonly Dictionary<int, List<object>> _locks = new Dictionary<int, List<object>>();

    public static ResourceHandler Instance {/* Singleton */}

    public virtual void Lock(int threadNumber, object obj)
    {
        Monitor.Enter(obj);

        if (!_locks.ContainsKey(threadNumber)) {_locks.Add(new List<object>());}
        _locks[threadNumber].Add(obj);
    }

    public virtual void Release(int threadNumber, object obj)
    {
       // Check whether we have threadN in _lock and skip if not
       var count = _locks[threadNumber].Count(x => x == obj);
       _locks[threadNumber].RemoveAll(x => x == obj);

       for (int i=0; i<count; i++)
       {
           Monitor.Exit(obj);
       }
    }

    // .....
 }

Actually what I am worried here about is thread-safety. I'm actually not sure, is it thread-safe or not, and it's a real pain to fix that. Am I doing the task correctly and how can I ensure that this is thread-safe?

Upvotes: 2

Views: 1305

Answers (4)

C.Evenhuis
C.Evenhuis

Reputation: 26446

Your Lock method locks on the target objects but the _locks dictionary can be accessed by any thread at any time. You may want to add a private lock object for accessing the dictionary (in both the Lock and Release methods).

Also keep in mind that by using such a ResourceHandler it is the responsibility of the rest of the code (the consuming threads) to release all used objects (a regular lock () block for instance covers that problem since whenever you leave the lock's scope, the object is released).

You also may want to use ReferenceEquals when counting the number of times an object is locked instead of ==.

Upvotes: 2

MoonKnight
MoonKnight

Reputation: 23831

What you have got conceptually looks dangerous; this is bacause calls to Monitor.Enter and Monitor.Exit for them to work as a Lock statement, are reccomended to be encapsulated in a try/finally block, that is to ensure they are executed sequetally. Calling Monitor.Exit before Monitor.Enter will throw an exception.

To avoid these problems (if an exception is thrown, the lock for a given thread may-or-may-not be taken, and if a lock is taken it will not be released, resulting in a leaked lock. I would recomend using one of the options provided in the other answers above. However, if you do want to progress with this mechanism, CLR 4.0 added the following overload to the Monitor.Enter method

public static void Enter (object, ref bool lockTaken);

lockTaken is false if and only if the Enter method throws an exception and the lock was not taken. So, using your two methods using a global bool lockTaken you can create something like (here the example is for a single locker - you will need a Dictionary of List<bool> corresponding to your threads - or event better a Tuple). So in your method Lock you would have something like

bool lockTaken = false;
Monitor.Enter(locker, ref lockTaken);

in the other method Release

if (lockTaken)
    Monitor.Exit(locker);

I hope this helps.

Edit: I don't think I fully appreciate your problem, but from what I can gather I would be using a Concurrent Collection. These are fully thead safe. Check out IProducerConsumerCollection<T> and ConcurrentBag<T>. These should facilitate what you want with all thread safter taken care of by the framework (note. a thread safe collection doesn't mean the code it executes is thread safe!). However, using a collection like this, is likely to be far slower than using locks.

Upvotes: 1

Jodrell
Jodrell

Reputation: 35746

You can ensure this class is thread safe by using a ConcurrentDictionary but, it won't help you with all the problems you will get from trying to develop your own locking mechanism.

There are a number locking mechansims that are already part of the .Net Framework, you should use those.

It sounds like you are going to need to use a combination of these, including Wait Handles to achieve what you want.

EDIT

After reading more carefully, I think you might need an EventWaitHandle

Upvotes: 1

Boris Ivanov
Boris Ivanov

Reputation: 4254

IMO you need to use atomic set of functions to make it safe.

http://msdn.microsoft.com/en-us/library/system.threading.mutex.aspx

Mutexes I guess will help u.

Upvotes: 0

Related Questions