CODEWITHSUNDEEP
c#concurrencydictionarylocking
Mikieeee
Mikieeee

Reputation:

C# concurrency, locking and dictionary objects

I have a bunch of DB entities that are loaded into DB objects. The same DB entity may be loaded into more than DB object. Periodically a DB entity will require special processing. This processing must be performed by one thread at a time. Locking is in order here.

EDIT: Important note: the process calls a slow web service. This is what I'm trying to prevent concurrency. I don't see how this can be done safely w/o locks.

So I create an “padlock” object that will be referenced by the DB objects for locking. The padlock object is entity based so that two or more DB objects for the same entity will use the same padlock object. I’m storing these padlocks in a dictionary object using the DB entity’s ID as the key. The padlock object is just a simple string object as well. Is this the right approach? I'm thinking I'm either over engineering or simplifying this. If the approach is correct, how does this code look? It works, but I've yet to test it under load.

Thanks :)

public static func(CustomObject o)
{
    if (ReadyForUpdate(o))
    {
        lock (LookupPadLockByID(object.ID)
        {
            if (ReadyForUpdate(o))
            {
                PerformUpdate(object);
            }
        }
    }
}

private static readonly object padlockLock = new object();
private static readonly Dictionary<int, string> padLocks = new Dictionary<int,string>();
private static object LookupPadLockByID(int uniqueID)
{
    lock (padlockLock)
    {
        if (!padLocks.ContainsKey(uniqueID))
        {
            padLocks.Add(uniqueID, uniqueID + " is locked");
        }
    }

    return padLocks[uniqueID];
}

Upvotes: 0

Views: 6354

Answers (4)

configurator
configurator

Reputation: 41690

Locking on a string is not a good idea. I suggest two alternatives:

  1. Use a Dictionary<int,object> as padLocks' type, and put a new object(); as the value.
  2. Create a class that simply holds the id; this would be better for readability if you ever want to look at your LockPad class while debugging.

LockPad class:

class LockPad {
    public int Id { get; private set; }

    public LockPad(int id) {
        this.Id = id;
    }

    public override string ToString() {
        return "lock of " + id.ToString();
    }
}

Then, lock on that class.

Upvotes: 1

user54650
user54650

Reputation: 4426

If you're using a standard Database of any type, I would suggest dumping these client-side locks entirely in favor of transactions and table/row locks.

Upvotes: 0

user53564
user53564

Reputation: 295

I think you are over engineering. If you only need to protect your DB entities (which I assume is represented by "object" in your code, which I will change to "entity"), you can just use it as your lock. Any reference object can be used as a lock:

public static func(CustomObject o)
{
    if (ReadyForUpdate(o))
    {
        lock (entity)
        {
                if (ReadyForUpdate(o))
                {
                        PerformUpdate(entity);
                }
        }
    }
}

Upvotes: 0

Marc Gravell
Marc Gravell

Reputation: 1064204

Well, you end up locking on a string, which isn't a good idea (although the concatenation means that interning shouldn't be a huge issue). What is a bigger issue is that:

  • you don't seem to remove the locks from padLocks - is that an issue?
  • you access the dictionary outside the padlockLock; the return should be inside the lock

For this second, this would be simpler:

object itemLock;
if (!padLocks.TryGetValue(uniqueID, out itemLock)) {
    itemLock = new object();
    padLocks.Add(uniqueID, itemLock);
}
return itemLock;

If this code is fairly local (i.e. your objects haven't escaped yet), you might simply lock the record itself? A lot simpler...

Upvotes: 1

Related Questions