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\$
  • \$\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 Aug 17 '12 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 Sep 22 '12 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 Jun 14 at 5:01
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\$
  • \$\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 Sep 22 '12 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\$
  • \$\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 Sep 22 '12 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 Sep 22 '12 at 10:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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