Locking by string. Is this safe/sane?

Question

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 } 

Answer 1

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).