Locks and exceptions do not mix

Locks and exceptions do not mix

Rate This
  • Comments 39

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!

  • 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. -- Eric

  • @configurator: yes, exactly, I do not see a good way to fix usings too.

  • > Locks are not magic dust that you sprinkle on code and suddenly it gets safe.

    Yeah, STM is that magic dust :)

  • 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, Jacco

  • I don't understand the dislike of ThreadAbortException.

    The way I see it, it's an ideal way of stopping an ongoing bit of work.  Sure, it may cause an exception where it is not expected; but so long as you presume that the thread-relevant state is trash anyhow, it matters little what state (or, for that matter locks) it held; those should be released.  

    This is contrary to both fact and the intended design of the feature. You should never abort a thread unless you are attempting to do an emergency shutdown of an appdomain. -- Eric

    Externally aborting a thread means it's possible to have simpler processing code (no checking for stopping flags), means it's possible to save execution resources if work-items are fairly granular (which they often are), and the drawbacks seem to apply when you cause such an exception and then actually expect the datastructures the thread was mutating to have a consistent state.

    I realize that ThreadAbort exceptions pose difficulty to the CLR designers, but there's a real advantage to their usage that cannot be replicated by means of setting some abort flag (if that's even possible - the code you want to abort may well be deep in external code - such as, for instance, other framework library code).

    Essentially, using "abort" flags represents fully executing the work item and then ignoring the result, whereas Thread.Abort represents trashing the current execution immediately.  The latter functionality isn't useless, and I don't see an alternative.

    If you need to take down a long-running unit of work that is full of code you do not control, a thread is the wrong way to structure that unit of work. We have work structures designed specifically for that scenario, namely processes and appdomains. If your application has the need to spawn off work in a way that the work can be cleanly killed halfway through regardless of what it is doing, you should be using the heavier-weight structures that were designed to facilitate that pattern. Using tools like threads for purposes that they were explicitly not designed for is a recipe for bugs. -- Eric

  • .NET ASP.NET Routing performance compare to HttpHandler LINQ on Objects Performance Why My ASP.NET Application

  • .NETASP.NETRoutingperformancecomparetoHttpHandlerLINQonObjectsPerformanceWhy...

  • Re using once again:

    There are a lot of people using disposable pattern (with usings) to implement "flag on/flag off" &c, so while it may not be the intended purpose, it is still quite conveninent and widespread.

    I am aware that this abuse is widespread. Being widespread does not make it a good idea. It is a bad idea to use our tools "off label" for purposes that they were never intended to be used for. And it should not be a surprise that when you do, you sometimes get unexpected bad behaviour. -- Eric

    But here is the other question: how does Thread.Abort work with finalizable objects? As in:

    var s = new StreamReader();

    ...

    what if thread abort happen inside the call to constructor?

    Constructors are nothing special to the thread manager. They're just methods that happen to be called right after an object is allocated. -- Eric

    Will object get to finalazer queue in that case?

    Of course. It had better -- the constructor might have already allocated objects that need finalization! -- Eric

    If it does, will that mean that finilizer will see uninitialized object?

    Yes. So you had better make sure that you write your finalizable objects to be robust in the situation where constructors fail half way through. Since you already have to write your finalizers to be robust in the face of multi-threading race conditions, this shouldn't be too much of an additional burden. (Finalizers run on a different thread than the main execution thread, so they have to be thread safe.) Remember, writing finalizable objects is an advanced topic for programmers who know exactly what the runtime finalization semantics are. If you do not know how it works, do what I do. I do not attempt to write a finalizer without help from someone who does understand it. -- Eric

  • @eric: yep, finalizers are difficult to get right :)

    I still do not feel that using the "using" in the way I desribed is a bad idea (well, as long as you do not abort your threads). Here is a nice article by Hans Boehm on a difference between destructors and finalizers (http://www.hpl.hp.com/techreports/2002/HPL-2002-335.html)

    Now, IDisposable.Dispose is a (sort of a) destructor (invoked synchronously when "static" lifetime of an object ends), and finalizer is, well, a finalizer.

    [Hans's C++ example for usability of destructors involves locks, but one can also imagine completely managed and harmless examples, e.g.: using(new EventListener(o)) { ... }, where EventListener subscribes to certain o's event in constructor and unsubscribes from the event in Dispose]

    I feel that C# does a very good job here in separating destructors and finalizers and giving the programmer a nice syntax for both - so I do not quite see why you consider it an abuse of the feature. I feel you are trying to take away a very nice and proper element of the language :)  - which not many other languages have btw!

    Of course, Thread.Abort kills all the nice properties of all that - and writing cleanly-abortable threads is difficult, and 99% of the code written is not cleanly abortable anyway.

    So I would say it is quite ok just to say that "using" is not cleanly abortable, while the "lock" with the new semantics is (with the caveats you have so clearly exposed in your post)

  • [Hans's C + +-Beispiel für die Nutzbarkeit von Destruktoren die Schlösser, aber man kann sich auch völlig harmlos und verwaltet Beispiele, zB: mit (neuen EventListener (o)) (... ), Wenn sich an bestimmten EventListener o Veranstaltung in Konstruktor und unsubscribes von der Veranstaltung in Entsorgen]

  • [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]

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

  • 외부에서 스레드를 중지 그걸 막을 플래그 (더 간단하게 처리하는 코드가 확인 가능), 수단 실행 자원 - 상품 경우 상당히 세분화하는 작업을 저장할 수있어 (자주)가 있고, 뜻 단점을 적용할 때 발생할 것 이런 예외를 실제로 스레드 datastructures 기대가 일관성있는 상태가 돌연변했다.

  • We have, in our program, a window that shows an insurance policy. The calculations needed to get its values are *extremely* complicated and take 2 or 3 minutes - but that didn't matter really. Until there was a window required that showed the results "online". This means that whenever the user changes a field, the results have to be recalculated (never mind that they take time to show, we use a progress bar and all).

    So what happens now is:

    The user changes a field. A thread is created for the calculation. Then the user changes another field. That thread is aborted, everything it did is completely irrelevant and is scrapped, and a new thread is created. If the user waits a while, the results are shown.

    This works perfectly at the moment, I might add.

    Since you're saying Thread.Abort is the wrong thing to do here, how would you do this? We are extremely reluctant to change the calculation method, since it involves a lot of Code Voodoo (really *really* bad code).

    Deciding what to do about that situation depends on the immediate and long-term business goals of your organization. I am reluctant to touch working, debugged code even if it is in many ways horrid unless doing so meets a long-term business goal.

    For example, if this code is likely to be modified a lot in the future, either because it is at present a bug farm, or because the policies which it implements are likely to change, then I'd consider carefully re-engineering the code so that it is (1) easier to understand and modify in the future and (2) actually engineered to fulfill the needs of the usage case. If the calculation code needs to be interruptable then design it to be interruptable and engineer interrupt points into it. Then you won't need to abort the thread; you'll have a mechanism that lets you shut it down cleanly.

    If, on the other hand, the code is fine as it is and there's no need to change it, then don't spend thousands of dollars of company money for no gain. Just add a comment saying that this has been identified as a bad programming practice, and consider fixing it in the future if things change. -- Eric

  • "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.

Page 2 of 3 (39 items) 123