Locking by string. Is this safe/sane?

I need to 开发者_运维技巧lock a section of code by string. Of course the following code is hideously unsafe:

lock("http://someurl")
{
    //bla
}

So I've been cooking up an alternative. I'm not normally one to post large bodies of code here, but when it comes to concurrent programming, I'm a little apprehensive about making my own synchronization scheme, so I'm submitting my code to ask if it's sane to do it in this way or whether there's a more straightforward approach.

public class StringLock
{
    private readonly Dictionary<string, LockObject> keyLocks = new Dictionary<string, LockObject>();
    private readonly object keyLocksLock = new object();

    public void LockOperation(string url, Action action)
    {
        LockObject obj;
        lock (keyLocksLock)
        {
            if (!keyLocks.TryGetValue(url,
                                      out obj))
            {
                keyLocks[url] = obj = new LockObject();
            }
            obj.Withdraw();
        }
        Monitor.Enter(obj);
        try
        {
            action();
        }
        finally
        {
            lock (keyLocksLock)
            {
                if (obj.Return())
                {
                    keyLocks.Remove(url);
                }
                Monitor.Exit(obj);
            }
        }
    }

    private class LockObject
    {
        private int leaseCount;

        public void Withdraw()
        {
            Interlocked.Increment(ref leaseCount);
        }

        public bool Return()
        {
            return Interlocked.Decrement(ref leaseCount) == 0;
        }
    }
}

I would use it like this:

StringLock.LockOperation("http://someurl",()=>{
    //bla
});

Good to go, or crash and burn?

EDIT

For posterity, here's my working code. Thanks for all the suggestions:

public class StringLock
{
    private readonly Dictionary<string, LockObject> keyLocks = new Dictionary<string, LockObject>();
    private readonly object keyLocksLock = new object();

    public IDisposable AcquireLock(string key)
    {
        LockObject obj;
        lock (keyLocksLock)
        {
            if (!keyLocks.TryGetValue(key,
                                      out obj))
            {
                keyLocks[key] = obj = new LockObject(key);
            }
            obj.Withdraw();
        }
        Monitor.Enter(obj);
        return new DisposableToken(this,
                                   obj);
    }

    private void ReturnLock(DisposableToken disposableLock)
    {
        var obj = disposableLock.LockObject;
        lock (keyLocksLock)
        {
            if (obj.Return())
            {
                keyLocks.Remove(obj.Key);
            }
            Monitor.Exit(obj);
        }
    }

    private class DisposableToken : IDisposable
    {
        private readonly LockObject lockObject;
        private readonly StringLock stringLock;
        private bool disposed;

        public DisposableToken(StringLock stringLock, LockObject lockObject)
        {
            this.stringLock = stringLock;
            this.lockObject = lockObject;
        }

        public LockObject LockObject
        {
            get
            {
                return lockObject;
            }
        }

        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        ~DisposableToken()
        {
            Dispose(false);
        }

        private void Dispose(bool disposing)
        {
            if (disposing && !disposed)
            {
                stringLock.ReturnLock(this);
                disposed = true;
            }
        }
    }

    private class LockObject
    {
        private readonly string key;
        private int leaseCount;

        public LockObject(string key)
        {
            this.key = key;
        }

        public string Key
        {
            get
            {
                return key;
            }
        }

        public void Withdraw()
        {
            Interlocked.Increment(ref leaseCount);
        }

        public bool Return()
        {
            return Interlocked.Decrement(ref leaseCount) == 0;
        }
    }
}

Used as follows:

var stringLock=new StringLock();
//...
using(stringLock.AcquireLock(someKey))
{
    //bla
}

Locking by an arbitrary string instance would be a bad idea, because Monitor.Lock locks the instance. If you had two different string instances with the same content, that would be two independent locks, which you don't want. So you're right to be concerned about arbitrary strings.

However, .NET already has a built-in mechanism to return the "canonical instance" of a given string's content: String.Intern. If you pass it two different string instances with the same content, you will get back the same result instance both times.

lock (string.Intern(url)) {
    ...
}

This is simpler; there's less code for you to test, because you'd be relying on what's already in the Framework (which, presumably, already works).

Another option is to get the HashCode of each URL, then divide it by a prime number and use it as an index into an array of locks. This will limit the number of locks you need while letting you control the probability of a “false locking” by choose the number of locks to use.

However the above is only worthwhile if it is too costly just have one lock per active url.

Here is a very simple, elegant and correct solution for .NET 4 using ConcurrentDictionary adapted from this question.

public static class StringLocker
{
    private static readonly ConcurrentDictionary<string, object> _locks = new ConcurrentDictionary<string, object>();

    public static void DoAction(string s, Action action)
    {
        lock(_locks.GetOrAdd(s, new object()))
        {
            action();
        }
    }
}

You can use this like so:

StringLocker.DoAction("http://someurl", () =>
{
    ...
});

About 2 years ago i had to implement the same thing:

I'm trying to write an image cache where multiple clients (over a 100 or so) are likely to request the image simultaneously. The first hit will request the image from another server, and I'd like all other requests to the cache to block until this operation is complete so that they can retrieve the cached version.

I ended up with code doing pretty the same as yours (the dictionary of LockObjects). Well, yours seems to be better encapsulated.

So, I think you have quite a good solution. Just 2 comments:

1. If you need even better peformance it maybe useful to utilize some kind of ReadWriteLock, since you have 100 readers and only 1 writer getting the image from another server.

2. I am not sure what happens in case of thread switch just before Monitor.Enter(obj); in your LockOperation(). Say, the first thread wanting the image constructs a new lock and then thread switch just before it enters critical section. Then it could happen that the second thread enters the critical section before the first. Well could be that this is not a real problem.

You've created a pretty complex wrapper around a simple lock statement. Wouldn't it be better to create a dictionary of url's and create a lock object for each and everyone. You could simply do.

objLink = GetUrl("Url"); //Returns static reference to url/lock combination
lock(objLink.LockObject){
    //Code goes here
}

You could even simplify this by locking the objLink object directly wich could be the GetUrl("Url") string instance. (you'd have to lock the static list of strings though)

You're original code if very error prone. If the code:

if (obj.Return())
{
keyLocks.Remove(url);
}

If the original finally code throws an exception you're left with an invalid LockObject state.

I added a solution in Bardock.Utils package inspired by @eugene-beresovsky answer.

Usage:

private static LockeableObjectFactory<string> _lockeableStringFactory = 
    new LockeableObjectFactory<string>();

string key = ...;

lock (_lockeableStringFactory.Get(key))
{
    ...
}

Solution code:

namespace Bardock.Utils.Sync
{
    /// <summary>
    /// Creates objects based on instances of TSeed that can be used to acquire an exclusive lock.
    /// Instanciate one factory for every use case you might have.
    /// Inspired by Eugene Beresovsky's solution: https://stackoverflow.com/a/19375402
    /// </summary>
    /// <typeparam name="TSeed">Type of the object you want lock on</typeparam>
    public class LockeableObjectFactory<TSeed>
    {
        private readonly ConcurrentDictionary<TSeed, object> _lockeableObjects = new ConcurrentDictionary<TSeed, object>();

        /// <summary>
        /// Creates or uses an existing object instance by specified seed
        /// </summary>
        /// <param name="seed">
        /// The object used to generate a new lockeable object.
        /// The default EqualityComparer<TSeed> is used to determine if two seeds are equal. 
        /// The same object instance is returned for equal seeds, otherwise a new object is created.
        /// </param>
        public object Get(TSeed seed)
        {
            return _lockeableObjects.GetOrAdd(seed, valueFactory: x => new object());
        }
    }
}

This is how I implemented this locking schema:

public class KeyLocker<TKey>
{
    private class KeyLock
    {
        public int Count;
    }

    private readonly Dictionary<TKey, KeyLock> keyLocks = new Dictionary<TKey, KeyLock>();

    public T ExecuteSynchronized<T>(TKey key, Func<TKey, T> function)
    {
        KeyLock keyLock =  null;
        try
        {              
            lock (keyLocks)
            {
                if (!keyLocks.TryGetValue(key, out keyLock))
                {
                    keyLock = new KeyLock();
                    keyLocks.Add(key, keyLock);
                }
                keyLock.Count++;
            }
            lock (keyLock)
            {
                return function(key);
            }
        }
        finally
        {         
            lock (keyLocks)
            {
                if (keyLock != null && --keyLock.Count == 0) keyLocks.Remove(key);
            }
        }
    }

    public void ExecuteSynchronized(TKey key, Action<TKey> action)
    {
        this.ExecuteSynchronized<bool>(key, k =>
        {
            action(k);
            return true;
        });
    }
}

And used like this:

private locker = new KeyLocker<string>();
......

void UseLocker()
{
     locker.ExecuteSynchronized("some string", () => DoSomething());
}

In most cases, when you think you need locks, you don't. Instead, try to use thread-safe data structure (e.g. Queue) which handles the locking for you.

For example, in python:

class RequestManager(Thread):
    def __init__(self):
        super().__init__()
        self.requests = Queue()
        self.cache = dict()
    def run(self):        
        while True:
            url, q = self.requests.get()
            if url not in self.cache:
                self.cache[url] = urlopen(url).read()[:100]
            q.put(self.cache[url])

    # get() runs on MyThread's thread, not RequestManager's
    def get(self, url):
        q = Queue(1)
        self.requests.put((url, q))
        return q.get()

class MyThread(Thread):
    def __init__(self):
        super().__init__()
    def run(self):
        while True:
            sleep(random())
            url = ['http://www.google.com/', 'http://www.yahoo.com', 'http://www.microsoft.com'][randrange(0, 3)]
            img = rm.get(url)
            print(img)

Now I have the same issue. I decided to use simple solution: to avoid deadlocks create a new string with fixed unique prefix and use it as a lock object:

lock(string.Intern("uniqueprefix" + url))
{
//
}