CODEWITHSUNDEEP
c#.net
user466512
user466512

Reputation: 147

Lock only on an Id

I have a method which needs to run exclusivley run a block of code, but I want to add this restriction only if it is really required. Depending on an Id value (an Int32) I would be loading/modifying distinct objects, so it doesn't make sense to lock access for all threads. Here's a first attempt of doing this -

private static readonly ConcurrentDictionary<int, Object> LockObjects = new ConcurrentDictionary<int, Object>();
void Method(int Id)
{
    lock(LockObjects.GetOrAdd(Id,new Object())
    {
       //Do the long running task here - db fetches, changes etc
       Object Ref;
       LockObjects.TryRemove(Id,out Ref);
    }

}

I have my doubts if this would work - the TryRemove can fail (which will cause the ConcurrentDictionary to keep getting bigger).

A more obvious bug is that the TryRemove successfully removes the Object but if there are other threads (for the same Id) which are waiting (locked out) on this object, and then a new thread with the same Id comes in and adds a new Object and starts processing, since there is no one else waiting for the Object it just added.

Should I be using TPL or some sort of ConcurrentQueue to queue up my tasks instead ? What's the simplest solution ?

Upvotes: 9

Views: 3487

Answers (4)

ekha
ekha

Reputation: 21

If you want to use the ID itself and do not allow collisions, caused by hash-code, you can you the next approach. Maintain the Dictionary of objects and store info about the number of the threads, that want to use ID:

class ThreadLockerByID<T>
{
    Dictionary<T, lockerObject<T>> lockers = new Dictionary<T, lockerObject<T>>();

    public IDisposable AcquireLock(T ID)
    {
        lockerObject<T> locker;
        lock (lockers)
        {
            if (lockers.ContainsKey(ID))
            {
                locker = lockers[ID];
            }
            else
            {
                locker = new lockerObject<T>(this, ID);
                lockers.Add(ID, locker);
            }
            locker.counter++;
        }
        Monitor.Enter(locker);
        return locker;
    }
    protected void ReleaseLock(T ID)
    {
        lock (lockers)
        {
            if (!lockers.ContainsKey(ID))
                return;

            var locker = lockers[ID];

            locker.counter--;

            if (Monitor.IsEntered(locker))
                Monitor.Exit(locker);

            if (locker.counter == 0)
                lockers.Remove(locker.id);
        }
    }

    class lockerObject<T> : IDisposable
    {
        readonly ThreadLockerByID<T> parent;
        internal readonly T id;
        internal int counter = 0;
        public lockerObject(ThreadLockerByID<T> Parent, T ID)
        {
            parent = Parent;
            id = ID;
        }
        public void Dispose()
        {
            parent.ReleaseLock(id);
        }
    }
}

Usage:

partial class Program
{
    static ThreadLockerByID<int> locker = new ThreadLockerByID<int>();
    static void Main(string[] args)
    {
        var id = 10;
        using(locker.AcquireLock(id))
        {

        }
    }
}

Upvotes: 1

ekha
ekha

Reputation: 21

I used the following approach. Do not check the original ID, but get small hash-code of int type to get the existing object for lock. The count of lockers depends on your situation - the more locker counter, the less the probability of collision.

class ThreadLocker
{
    const int DEFAULT_LOCKERS_COUNTER = 997;
    int lockersCount;
    object[] lockers;

    public ThreadLocker(int MaxLockersCount)
    {
        if (MaxLockersCount < 1) throw new ArgumentOutOfRangeException("MaxLockersCount", MaxLockersCount, "Counter cannot be less, that 1");
        lockersCount = MaxLockersCount;
        lockers = Enumerable.Range(0, lockersCount).Select(_ => new object()).ToArray();
    }
    public ThreadLocker() : this(DEFAULT_LOCKERS_COUNTER) { }

    public object GetLocker(int ObjectID)
    {
        var idx = (ObjectID % lockersCount + lockersCount) % lockersCount;
        return lockers[idx];
    }
    public object GetLocker(string ObjectID)
    {
        var hash = ObjectID.GetHashCode();
        return GetLocker(hash);
    }
    public object GetLocker(Guid ObjectID)
    {
        var hash = ObjectID.GetHashCode();
        return GetLocker(hash);
    }
}

Usage:

partial class Program
{
    static ThreadLocker locker = new ThreadLocker();
    static void Main(string[] args)
    {
        var id = 10;
        lock(locker.GetLocker(id))
        {

        }
    }
}

Of cource, you can use any hash-code functions to get the corresponded array index.

Upvotes: 1

Emond
Emond

Reputation: 50672

The main semantic issue I see is that an object can be locked without being listed in the collection because the the last line in the lock removes it and a waiting thread can pick it up and lock it.

Change the collection to be a collection of objects that should guard a lock. Do not name it LockedObjects and do not remove the objects from the collection unless you no longer expect the object to be needed.

I always think of this type of objects as a key instead of a lock or blocked object; the object is not locked, it is a key to locked sequences of code.

Upvotes: 1

Dave Lawrence
Dave Lawrence

Reputation: 3929

I use a similar approach to lock resources for related items rather than a blanket resource lock... It works perfectly.

Your almost there but you really don't need to remove the object from the dictionary; just let the next object with that id get the lock on the object.

Surely there is a limit to the number of unique ids in your application? What is that limit?

Upvotes: 3

Related Questions