Locks and exceptions do not mix
A couple years ago I wrote a bit about how our codegen for the lock statement could sometimes lead to situations in which an unoptimized build had different potential deadlocks than an optimized build of the same source code. This is unfortunate, so we've fixed that for C# 4.0. However, all is still not rainbows, unicorns and Obama, as we'll see.
Recall that lock(obj){body} was a syntactic sugar for
var temp = obj;
Monitor.Enter(temp);
try { body }
finally { Monitor.Exit(temp); }
The problem here is that if the compiler generates a no-op instruction between the monitor enter and the try-protected region then it is possible for the runtime to throw a thread abort exception after the monitor enter but before the try. In that scenario, the finally never runs so the lock leaks, probably eventually deadlocking the program. It would be nice if this were impossible in unoptimized and optimized builds.
In C# 4.0 we've changed lock so that it now generates code as if it were
bool lockWasTaken = false;
var temp = obj;
try { Monitor.Enter(temp, ref lockWasTaken); { body } }
finally { if (lockWasTaken) Monitor.Exit(temp); }
The problem now becomes someone else's problem; the implementation of Monitor.Enter takes on responsibility for atomically setting the flag in a manner that is immune to thread abort exceptions messing it up.
So everything is good now, right?
Sadly, no. It's consistently bad now, which is better than being inconsistently bad. But there's an enormous problem with this approach. By choosing these semantics for "lock" we have made a potentially unwarranted and dangerous choice on your behalf; implicit in this codegen is the belief that a deadlocked program is the worst thing that can happen. That's not necessarily true! Sometimes deadlocking the program is the better thing to do -- the lesser of two evils.
The purpose of the lock statement is to help you protect the integrity of a mutable resource that is shared by multiple threads. But suppose an exception is thrown halfway through a mutation of the locked resource. Our implementation of lock does not magically roll back the mutation to its pristine state, and it does not complete the mutation. Rather, control immediately branches to the finally, releasing the lock and allowing every other thread that is patiently waiting to immediately view the messed-up partially mutated state! If that state has privacy, security, or human life and safety implications, the result could be very bad indeed. In that case it is possibly better to deadlock the program and protect the messed-up resource by denying access to it entirely. But that's obviously not good either.
Basically, we all have a difficult choice to make whenever doing multi-threaded programming. We can (1) automatically release locks upon exceptions, exposing inconsistent state and living with the resulting bugs (bad) (2) maintain locks upon exceptions, deadlocking the program (arguably often worse) or (3) carefully implement the bodies of locks that do mutations so that in the event of an exception, the mutated resource is rolled back to a pristine state before the lock is released. (Good, but hard.)
This is yet another reason why the body of a lock should do as little as possible. Usually the rationale for having small lock bodies is to get in and get out quickly, so that anyone waiting on the lock does not have to wait long. But an even better reason is because small, simple lock bodies minimize the chance that the thing in there is going to throw an exception. It's also easier to rewrite mutating lock bodies to have rollback behaviour if they don't do very much to begin with.
And of course, this is yet another reason why aborting a thread is pure evil. Try to never do so!
Comments
Anonymous
March 06, 2009
Great post, but a scary topic. I can't help feel that the "take-away" from all this is to not use the C# lock keyword in production code. It seems like it is broken and isn't getting fixed really. No, that's not the takeaway. The takeaway is that locking is only a starting point in ensuring thread safety. Ensuring that the lock body is truly atomic is hard. Locks are not magic dust that you sprinkle on code and suddenly it gets safe. -- EricAnonymous
March 06, 2009
Since the only situation this change affects is when an exception is thrown immediately after Monitor.Enter and before the try is entered, which also means before the protected state has been mutated, I don't see how this change implies any particular opinion about deadlocking versus state inconsistency. Perhaps I was unclear. This change does not imply any such opinion. The whole design of the lock statement implies that opinion. This change does not affect the more general problem, which was my point. -- Eric More generally, isn't the issue of an exception resulting in partially mutated state orthogonal to the issue of locking? The same situation would be true if there were not threading or locking involved, and an exception is thrown while mutating state. Anything which tries to read the partially mutated state after the exception is thrown could be in trouble. That's why the general guidance on handling exceptions is, if the process might be executing in an unknown state, let it die. Yes, you have similar problems in single-threaded land, but it is harder to avoid the problem in mutli-threaded land. In a single-threaded situation, someone up the call stack can catch the exception and fix up the state if need be, or cleanly shut down the process before touching it again, or whatever. In a multi-threaded situation, the very moment the lock is unlocked, some thread could immediately be in there messing with the state. -- EricAnonymous
March 06, 2009
The comment has been removedAnonymous
March 06, 2009
ihmo, the issue is not how the Lock statement is implemented, but rather is about why you should never use the ThreadAbortException. Using the Abort method to stop a thread seems like a terrible thing to do. Think about it - you stop a thread by arbitrarily triggering an exception in it, without underlying cause. Therefore, the answer is, avoid using Thread.Abort like the plague. If you need to be able to stop a thread, define a boolean flag, set it to abort the thread, and let the thread abort itself in a proper fashion, by checking the flag and shutting itself down cleanly, under its own control. I have done multithreaded client and server code often, and never needed Thread.Abort. Without Thread.Abort, the lock statement quirk described becomes a nonissue. Ultimately, I think thread.Abort and the ThreadAbortException should just be eliminated from the framework to solve the problem. But I agree with the design decision that a deadlock is the worst thing to happen. I do not know of a case where it is not. In server side apps, it means you have the whole IT department on your back, since they have nothing better to do than count server reboots, and for a client application, nothing is more exasperating for a user than an app that just freezes and sits there, needing to be end tasked. Indeed, thread abort exceptions are pure unmitigated evil and you should never use them. -- EricAnonymous
March 06, 2009
I've got to agree with the other commenters. The lock statement does one thing and one thing only; ensure that the monitor enter and monitor exit calls are matched properly. That is getting fixed. Making sure all the application code is exception neutral is a completely separate issue.Anonymous
March 06, 2009
I also agree with others. It's the responsibility of the programmer to ensure appropriate exception safety of the code. lock() must only ensure that resource is unlocked when some exception is thrown within its bodyAnonymous
March 06, 2009
The comment has been removedAnonymous
March 06, 2009
The comment has been removedAnonymous
March 06, 2009
Will the C#4.0 fix also allow a TimeSpan or milliseconds to be passed in as a parameter to Enter so that an atomic setting of the lockWasTaken flag works when specifying a timeout? It would be great to have a new version of the lock statement like below: lock (obj, timeout) { lockbody } else { timeoutbody } Sugar Coating for: bool lockWasTaken = false;
var temp = obj;
try {
Monitor.Enter(temp, timeout, ref lockWasTaken);
if (lockWasTaken) { lockbody } else { timeoutbody };
}
finally { if (lockWasTaken) Monitor.Exit(temp); } Sadly, no. I agree that this would be nice, but the cost of the feature doesn't pay for the benefit of the sugar. -- EricAnonymous
March 06, 2009
The comment has been removedAnonymous
March 06, 2009
I understand that the sugar coating was just pie in the sky, but will an Monitor.Enter or Monitor.TryEnter function be available to get an atomic setting of the lockWasTaken flag when passing in a TimeSpan or milliseconds? I wouldn't think that would take too much time to implement.Anonymous
March 06, 2009
The comment has been removedAnonymous
March 06, 2009
Since you mention the ThreadAbortException, what happens when: try {
Monitor.Enter(temp, ref lockWasTaken);
{ body }
}
finally
{
if (lockWasTaken)
// <---- The exception happens here
Monitor.Exit(temp);
} Good question. In CLR v2 and higher the exception is delayed until the finally block is done. Which means that the thread might never end at all. Which is another reason why attempting to abort a thread is pure evil. No guarantee that it will actually work. -- EricAnonymous
March 06, 2009
Just as an aside, javac does monitorenter try { body } catch (Throwable t) { monitorexit throw t; } monitorexit which is the same as in a previous post, and seems a lot more sane to me.Anonymous
March 07, 2009
Isn't using also a problem? As in: using(var s = new StreamReader()) {...} which translates into var s = new StreamReader();
try { ... } finally { s.Dispose(); } and now you have the same issue. (I've seen people using IDisposable for locks also quite bit actually). Any plans to address this? Suppose an exception is thrown after the new but before the try. What's the worst thing that can happen? The disposer does not run, and the garbage collector eventually cleans up the resource a while later. Remember, "using" was designed to allow you to more easily optimize the cleanup of scarce resources. If a thread abort happens to un-optimize that process, it's probably not a big deal. (And if you are using "using" for something other than optimizing the cleanup of a scarce resource, then you're using it off-label. I recommend that you do not use "using" to implement locking semantics.) -- EricAnonymous
March 07, 2009
Using is even more problematic, since in the constructor anything can happen. Let's suppose that using was translated to: StreamReader s;
try {
s = new StreamReader();
...
} finally {
if (s != null)
s.Dispose();
} What if there's a ThreadAbortException after the resource is taken but before s is assigned? I see no way to fix that issue, actually. You don't have to fix it. The GC will eventually release the resource. -- Eric Eric: Does that delay happen for any finally block or only for a lock's finally block? Any finally block. Locks do not generate special IL. -- Eric Also, of course there's no guarantee! I had once a loop like this: while (!CalculationComplete()) {
try {
CalculateNextPart();
this.Invoke(UpdateProgressBar);
} catch (Exception e) {
ExceptionHandler.Handle(e); // ExceptionHandler logs the message and usually shows an error message but not in this specific case (client requirements)
}
} Note that any exception is caught and logged - including a ThreadAbortException. There was no way to actually abort that thread. only to abort a specific part of the calculation. Actually, I believe you cannot stop the thread abort exception. You can delay it indefinitely, but you cannot eat it and keep on trucking. If you try, you'll find that the thread does in fact go down. In the next version of the CLR there will be even more "inedible" exceptions. -- EricAnonymous
March 07, 2009
@configurator: yes, exactly, I do not see a good way to fix usings too.Anonymous
March 07, 2009
> Locks are not magic dust that you sprinkle on code and suddenly it gets safe. Yeah, STM is that magic dust :)Anonymous
March 07, 2009
Hi, I am a bit disappointed. I actually thought the ThreadAbortException was a pretty genious solution to finish infinitely working threads (I actually am still using this :s). I assumed (wrongly I suppose) that the .NET Framework would take care of the tricky parts of doing something as dangerous as that. Sorry to disappoint, but this programming practice is dangerous and directly contrary to good programming practices. The only reason you should ever abort a thread is when you are doing an emergency shutdown and are trying to take down the entire appdomain. Design your multi-threaded applications so that threads can be taken down cleanly. -- Eric A lot of threads I use look something like this: public void DoWork(object param) {
WorkItem workItem;
while (true) {
try {
lock(someQueue) {
workItem = someQueue.Dequeue();
}
if (workItem != null) {
// repeated payload of this thread
} else {
Thread.Sleep(15 * 1000);
}
} catch (Exception e) {
// reraise any exxceptions I really not should try doing something about
if (e is ThreadAbortException || e is OutOfMemoryException || e is StackOverflowException)
throw e;
// cleanup no reraiseunless decided fatal
// ...
}
}
} In future versions of the CLR, these exceptions will probably be "inedible" -- you will not need to re-throw them. They'll automatically be re-thrown if you attempt to handle them and continue. -- Eric To stop these creatures I use the Thread.Abort abort (which throws the ThreadAbortException. Using Intellisense this seemed the right way to go. I know I should be using events in stead of an ever repeating while loop but somehow I find this set up easier to grasp. Wouldn't it be nice if the would Framework not immediately stop but only in places where the TheadAbortException is actually caught (and maybe rethrown)? Regards, JaccoAnonymous
March 08, 2009
The comment has been removedAnonymous
March 08, 2009
.NET ASP.NET Routing performance compare to HttpHandler LINQ on Objects Performance Why My ASP.NET ApplicationAnonymous
March 08, 2009
.NETASP.NETRoutingperformancecomparetoHttpHandlerLINQonObjectsPerformanceWhy...Anonymous
March 09, 2009
The comment has been removedAnonymous
March 09, 2009
The comment has been removedAnonymous
March 10, 2009
The comment has been removedAnonymous
March 10, 2009
[Hans du C + + par exemple pour la facilité d'utilisation des destructeurs des écluses, mais on peut aussi imaginer complètement inoffensifs géré et exemples, par exemple: en utilisant (nouveau EventListener (o)) (... ), Où EventListener souscrit à certaines o dans le constructeur de l'événement et se désabonne de la manifestation en Eliminer]Anonymous
March 10, 2009
Annuler un thread extérieur signifie qu'il est possible de faire plus simple de transformation de code (pas de contrôle pour l'arrêt des drapeaux), signifie qu'il est possible d'économiser des ressources si l'exécution du travail sont assez granuleux (qui sont souvent), et les inconvénients semblent s'appliquer lorsque vous cause une telle exception, et puis en fait attendre le datastructures le fil est en mutation à un état cohérent.Anonymous
March 10, 2009
외부에서 스레드를 중지 그걸 막을 플래그 (더 간단하게 처리하는 코드가 확인 가능), 수단 실행 자원 - 상품 경우 상당히 세분화하는 작업을 저장할 수있어 (자주)가 있고, 뜻 단점을 적용할 때 발생할 것 이런 예외를 실제로 스레드 datastructures 기대가 일관성있는 상태가 돌연변했다.Anonymous
March 10, 2009
The comment has been removedAnonymous
March 10, 2009
"Actually, I believe you cannot stop the thread abort exception." I'm sure I did it somehow, without expecting it. I'm not sure how, though - this was years ago.Anonymous
March 10, 2009
The comment has been removedAnonymous
March 10, 2009
The comment has been removedAnonymous
March 12, 2009
"...but it's not all rainbows, unicorns, and Obama..." Haha! Thanks for that.Anonymous
March 12, 2009
I am probably going to receive some serious hate mail for this blog posting. As the years have gone by, I've grown to believe that exceptions were a laudable idea gone almost entirely awry. Perhaps there is some esoteric programming...Anonymous
March 13, 2009
By "rainbows, unicorns and Obama" are you perhaps referring to this? http://iamchriscollins.com/badpaintingsofbarackobama/images/4.jpgAnonymous
March 20, 2009
The comment has been removedAnonymous
April 30, 2009
The comment has been removedAnonymous
November 09, 2009
Regarding the comment earlier on this thread regarding transactional memory..... I was involved in a project that achieved this nearly 20 years ago, with acceptable performance (ie you could not statistically measure any performance difference using the transactional memory and normal memory) Of course I had one BIG advantage....it was custom HARDWARE that implemented the transactional memory. Great post on while lock(...) did not and does not, and probably never will magically make a system "safe"..on conventional hardware..Anonymous
August 27, 2012
If I'm understanding you correctly you are saying attempt the lock but return it to its previous state as a sort of roll back mechanism? public void Process(Car car, string make, string model) { var originalCar = car; var lockObj = new object(); try { lock (lockObj) { car.Make = make; car.Model = model; } } catch { //an error occured //return car to previous state car = originalCar; } finally { } }
