11
\$\begingroup\$

I'd like to use lock objects that are specific to the person I'm updating. In other words, if thread A is updating Person 1, thread B is blocked from also updating Person 1, but thread C is not blocked from updating Person 2.

99% of the time, I don't really need the locks since I'm working with different Person.Id values. In the occasional situations when I am working with the same Person.Id value, I want to lock around some read/write code.

The following code is working as I expect. I'm looking for any "gotcha's" I may have missed, or better ways of accomplishing the same thing. I decided to use the .NET Cache to store the lock objects so I don't need to worry about cleaning them up later (the lock objects are removed from cache when they haven't been used for a certain TimeSpan).

This code provides the object to lock on

public static class PersonLocks
{
    private static readonly object CacheLock = new object();
    private const string KeyPrefix = "LockForPersonID:";

    public static object GetPersonLock(long personId)
    {
        lock (CacheLock)
        {
            string key = BuildCacheKey(personId);
            object cachedItem = HttpRuntime.Cache[key];
            if (cachedItem == null)
            {
                cachedItem = new object();
                HttpRuntime.Cache.Insert(key, cachedItem, null, Cache.NoAbsoluteExpiration, new TimeSpan(0, 5, 0));
            }

            return cachedItem;
        }
    }

    private static string BuildCacheKey(long personId)
    {
        return KeyPrefix + personId.ToString();
    }

}

This is how I'm using the code

object padlock = PersonLocks.GetPersonLock(person.Id);
lock (padlock)
{
    //do read

    //do some data mapping

    //do write
}
\$\endgroup\$
3
  • \$\begingroup\$ Are the person objects stored in some data structure that all threads have access to? If they are, then can't you just lock the person object itself? \$\endgroup\$
    – pstrjds
    Commented Aug 17, 2012 at 19:27
  • \$\begingroup\$ No, the person objects have a very short lifetime, so they're not stored in a common data structure. I think your suggestion would work, but I generally prefer to use separate objects to lock on in order to avoid confusion about what the lock keyword is doing. \$\endgroup\$
    – tgriffin
    Commented Sep 22, 2012 at 0:10
  • \$\begingroup\$ @tgriffin Acquiring an outer lock lock (CacheLock) to get a person lock is expensive. I would use an atomic operation to get the person lock, rather than a lock. ConcurrentDictionaryis the way to go for me. The only downside is that you have to implement expiration yourself. \$\endgroup\$
    – dfhwze
    Commented Jun 14, 2019 at 5:01

2 Answers 2

5
\$\begingroup\$

Your code would do just fine, in terms of functionality.

However, note that such design holds as long as everybody knows that they should acquire the lock before performing any process on the person object.

Perhaps you should consider re-design your data objects (such as the person class) so that there's an explicit method for any update process. Perhaps something like BeginEdit and EndEdit, or, even better, a method that returns an IDisposable object that acquires the lock and releases it in its Dispose implementation. Example:

public class Person {
    public IDisposable AcquireLock() {
        // ...
    }

    // ...
}

// usage:
var aPerson = GetPerson(1234);
using (aPerson.AcquireLock()) {
    aPerson.LastName = "abcd";
    SavePerson(aPerson);
    // ...
}

This way the locking is made more explicit.

Just a suggestion, though...

\$\endgroup\$
1
  • \$\begingroup\$ I like this. I've upvoted your answer. I implemented the original code, but will consider doing something like this in the future. \$\endgroup\$
    – tgriffin
    Commented Sep 22, 2012 at 0:11
4
\$\begingroup\$

Use double check lock:

public static object GetPersonLock(long personId) 
{ 
        var key = BuildCacheKey(personId);
        object cachedItem = HttpRuntime.Cache[key];

        if (cachedItem != null) {
            return cachedItem;
        }

    lock (CacheLock) 
    {
            cachedItem = HttpRuntime.Cache[key];
            if (cachedItem != null) {
                return cachedItem;
            }

        cachedItem = new object(); 
        HttpRuntime.Cache.Insert(key, cachedItem, null, Cache.NoAbsoluteExpiration, new TimeSpan(0, 5, 0)); 

        return cachedItem; 
    } 
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks. I've upvoted your answer. I considered doing this, but decided not to since 99% of the time the lock object won't be in cache (updates for the same person normally come infrequently, but on rare occassions I'll get 2 updates within a few milliseconds). I'm probably not saving much time, but decided to skip the initial "unlocked" read from cache. \$\endgroup\$
    – tgriffin
    Commented Sep 22, 2012 at 0:16
  • \$\begingroup\$ If you are not using correct locking you have big chance to end up within a deadlock situation. \$\endgroup\$
    – Peter Kiss
    Commented Sep 22, 2012 at 10:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.