(1 item) |
|
(1 item) |
|
(5 items) |
|
(1 item) |
|
(1 item) |
|
(2 items) |
|
(2 items) |
|
(4 items) |
|
(1 item) |
|
(6 items) |
|
(2 items) |
|
(4 items) |
|
(1 item) |
|
(4 items) |
|
(2 items) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(2 items) |
|
(2 items) |
|
(5 items) |
|
(3 items) |
|
(1 item) |
|
(1 item) |
|
(1 item) |
|
(3 items) |
|
(1 item) |
|
(1 item) |
|
(2 items) |
|
(8 items) |
|
(2 items) |
|
(7 items) |
|
(2 items) |
|
(2 items) |
|
(1 item) |
|
(2 items) |
|
(1 item) |
|
(2 items) |
|
(4 items) |
|
(1 item) |
|
(5 items) |
|
(1 item) |
|
(3 items) |
|
(2 items) |
|
(2 items) |
|
(8 items) |
|
(7 items) |
|
(3 items) |
|
(7 items) |
|
(6 items) |
|
(1 item) |
|
(2 items) |
|
(5 items) |
|
(5 items) |
|
(7 items) |
|
(3 items) |
|
(7 items) |
|
(16 items) |
|
(10 items) |
|
(27 items) |
|
(15 items) |
|
(15 items) |
|
(13 items) |
|
(16 items) |
|
(15 items) |
I have issues with the lock
keyword in C#. It is designed to make writing thread-safe code easier,
but I never use it in production code.
In case you're not familiar with it, lock
is just shorthand - the following code:
lock (obj)
{
... do something with shared state ...
}
is expanded by the compiler into this:
Monitor.Enter(obj); try { ... do something with shared state ... } finally { Monitor.Exit(obj); }
The reason lock
does this is that it's vitally important for any call to Monitor.Enter
to be
balanced out with a call to Monitor.Exit
. If you ever forget to do this, your code will acquire a monitor that
it never releases, which is likely to bring your program to a grinding halt pretty promptly. Putting the
Monitor.Exit
in a finally
block is a good way of ensuring that it is called, and the lock
keyword is a convenient way of doing this. And that's a good thing - it standardises a good practice, removes clutter from your
source code, and makes for an instantly recognizable idiom. (It's obvious when you look at a lock
block that something is
being done while in possession of a monitor. That's not quite so immediately apparent from the expanded version; it's
a small difference but small things like this make a surprising improvement to how quickly you can understand the
code you are looking at.)
This isn't the part I have a problem with.
The problem I have with lock
is that it calls Monitor.Enter
instead of
Monitor.TryEnter
.
What's wrong with Monitor.Enter
? The problem is that it will block indefinitely if the monitor is
unavailable. I avoid writing production code that calls APIs which might block forever, because if you hit a deadlock
scenario, you'll never recover. And even though you will normally want to design your code in such a way as to
avoid deadlock, it's still best to be able to at least detect it if it should occur. (Ideally you want to be robust enough to
recover from it too.)
The solution to this is to use Monitor.TryEnter
instead - this allows you to pass in a timeout,
specifying how long you're prepared to wait. The right timeout will depend on your application, but you will normally
be able to pick a timeout which, if exceeded, is clear evidence of deadlock. Most of the time I use something on the
order of 10 seconds - I normally try to hold locks for no more than a few statements, so if one has been unavailable
for 10 seconds, then things are clearly very badly wrong.
Unfortunately, because lock
uses Monitor.Enter
instead of
Monitor.TryEnter
, you can't use lock
blocks with a timeout. And it's understandable -
you have to add a block for the failure case, and it's not clear where that would go. Look at this example:
if (!Monitor.TryEnter(obj, TimeSpan.FromSeconds(10)) { throw new ApplicationException("Timeout waiting for lock"); } try { ... do something with shared state ... } finally { Monitor.Exit(obj); }
There's some error handling code in that if
block which has nowhere to go if you use lock
. I
suppose Microsoft could have added a second block. After all, if
statements support two blocks - one after
the if
and one after the else
. A similar construct for locks would have been possible
for them to add. For example, they could have done something like iflock
and then an else
block for the case where the lock couldn't be acquired,
but no such thing exists in practice.
However, it's not that much of a problem. There's a trick I've been using for a while which gives you much the same effect, but googling just now I didn't find any examples of this technique on the net, so I thought I'd write about it here.
The main problems with the example above are that it's a bit verbose in comparison with using the
lock
keyword, and it embeds the timeout throughout the code, which makes it awkward if you
decide you want to change it. (And even if you don't think you will, these timeouts clutter the code.)
The approach I use is a variant on a popular technique with IDisposable
and using
,
which in turn is modelled on the good old Resource
Acquisition is Initialization C++ idiom. The idea is to rely on the compiler's ability to free up objects when variables
go out of scope. In C++ you can use destructors to do this, but of course C# destructors aren't scope-based - they
are finalizers in disguise. But C# does let you use a using
statement to call an object's Dispose
method at the end of a scope, which we can exploit in the same way as we can exploit a C++ destructor.
This example shows the technique [update - now with improvements thanks to Eric Gunnerson]:
using System; using System.Threading; // Thanks to Eric Gunnerson for recommending this be a struct rather // than a class - avoids a heap allocation. // (In Debug mode, we make it a class so that we can add a finalizer // in order to detect when the object is not freed.) // Thanks to Chance Gillespie and Jocelyn Coulmance for pointing out // the bugs that then crept in when I changed it to use struct... #if DEBUG public class TimedLock : IDisposable #else public struct TimedLock : IDisposable #endif { public static TimedLock Lock (object o) { return Lock (o, TimeSpan.FromSeconds (10)); } public static TimedLock Lock (object o, TimeSpan timeout) { TimedLock tl = new TimedLock (o); if (!Monitor.TryEnter (o, timeout)) { #if DEBUG System.GC.SuppressFinalize(tl); #endif throw new LockTimeoutException (); } return tl; } private TimedLock (object o) { target = o; } private object target; public void Dispose () { Monitor.Exit (target); // It's a bad error if someone forgets to call Dispose, // so in Debug builds, we put a finalizer in to detect // the error. If Dispose is called, we suppress the // finalizer. #if DEBUG GC.SuppressFinalize(this); #endif } #if DEBUG ~TimedLock() { // If this finalizer runs, someone somewhere failed to // call Dispose, which means we've failed to leave // a monitor! System.Diagnostics.Debug.Fail("Undisposed lock"); } #endif } public class LockTimeoutException : Exception { public LockTimeoutException () : base("Timeout waiting for lock") { } }
The TimedLock.Lock
method can be called to lock an object. It returns an object that implements
IDisposable
. (As it happens, this is an instance of the TimedLock
class, but it doesn't
really matter what it is - a private nested class could also have been used.) Now we can write code like this:
using (TimedLock.Lock(obj))
{
}
This is a very similar usage model to using the lock
keyword - we rely on a compiler-generated
finally
block to call Monitor.Exit
. It's just a bit less direct - the generated block
calls Dispose
on the TimedLock
, which in turn calls Monitor.Exit
.
But now, if the monitor cannot be acquired (e.g., because of a deadlock) the code will throw an exception after
ten seconds instead of blocking for ever. We get pretty close to the succinctness of the lock
keyword, but we get to use Monitor.TryEnter
.
[Update: Eric Gunnerson has posted a response to
this in his blog. He points out that
you can improve this slightly by making TimedLock a struct
. This avoids a heap allocation. If making
this change it's also important to make the Dispose
method public, as otherwise, the
struct
will get boxed when IDisposable.Dispose
is called. I have updated the code
in this blog in accordance with his recommendations.]
[Update 2: Apparently I didn't look hard enough. Dimitri Glazkov posted a very similar example in his blog recently which I failed to find when googling!]
[Update 3: Chance Gillespie pointed out that if an exception is thrown, the TimedLock
won't
get disposed, so in Debug builds you'll get an assertion further down the line. So I modified the code to suppress
finalization if the lock acquisition fails. Jocelyn Coulmance also pointed out that I hadn't quite got the struct version
correct - I was still returning IDisposable
, which would result in the TimedLock
struct
getting boxed, defeating the whole point of using a struct in the first place!]
[Update 4: See more recent entries: ReaderWriterLock vs Monitor and Oh No! Not the TimedLock Again!]