Does a locked object stay locked if an exception occurs inside it?

Question

In a c# threading app, if I were to lock an object, let us say a queue, and if an exception occurs, will the object stay locked? Here is the pseudo-code:

int ii;
lock(MyQueue)
{
   MyClass LclClass = (MyClass)MyQueue.Dequeue();
   try
   {
      ii = int.parse(LclClass.SomeString);
   }
   catch
   {
     MessageBox.Show("Error parsing string");
   }
}

As I understand it, code after the catch doesn't execute - but I have been wondering if the lock will be freed.

Solution 1

First; have you considered TryParse?

in li;
if(int.TryParse(LclClass.SomeString, out li)) {
    // li is now assigned
} else {
    // input string is dodgy
}

The lock will be released for 2 reasons; first, lock is essentially:

Monitor.Enter(lockObj);
try {
  // ...
} finally {
    Monitor.Exit(lockObj);
}

Second; you catch and don't re-throw the inner exception, so the lock never actually sees an exception. Of course, you are holding the lock for the duration of a MessageBox, which might be a problem.

So it will be released in all but the most fatal catastrophic unrecoverable exceptions.

Solution 2

I note that no one has mentioned in their answers to this old question that releasing a lock upon an exception is an incredibly dangerous thing to do. Yes, lock statements in C# have "finally" semantics; when control exits the lock normally or abnormally, the lock is released. You're all talking about this like it is a good thing, but it is a bad thing! The right thing to do if you have a locked region that throws an unhandled exception is to terminate the diseased process immediately before it destroys more user data, not free the lock and keep on going.

Look at it this way: suppose you have a bathroom with a lock on the door and a line of people waiting outside. A bomb in the bathroom goes off, killing the person in there. Your question is "in that situation will the lock be automatically unlocked so the next person can get into the bathroom?" Yes, it will. That is not a good thing. A bomb just went off in there and killed someone! The plumbing is probably destroyed, the house is no longer structurally sound, and there might be another bomb in there. The right thing to do is get everyone out as quickly as possible and demolish the entire house.

I mean, think it through: if you locked a region of code in order to read from a data structure without it being mutated on another thread, and something in that data structure threw an exception, odds are good that it is because the data structure is corrupt. User data is now messed up; you don't want to try to save user data at this point because you are then saving corrupt data. Just terminate the process.

If you locked a region of code in order to perform a mutation without another thread reading the state at the same time, and the mutation throws, then if the data was not corrupt before, it sure is now. Which is exactly the scenario that the lock is supposed to protect against. Now code that is waiting to read that state will immediately be given access to corrupt state, and probably itself crash. Again, the right thing to do is to terminate the process.

No matter how you slice it, an exception inside a lock is bad news. The right question to ask is not "will my lock be cleaned up in the event of an exception?" The right question to ask is "how do I ensure that there is never an exception inside a lock? And if there is, then how do I structure my program so that mutations are rolled back to previous good states?"

Solution 3

yes, that will release properly; lock acts as try/finally, with the Monitor.Exit(myLock) in the finally, so no matter how you exit it will be released. As a side-note, catch(... e) {throw e;} is best avoided, as that damages the stack-trace on e; it is better not to catch it at all, or alternatively: use throw; rather than throw e; which does a re-throw.

If you really want to know, a lock in C#4 / .NET 4 is:

{
    bool haveLock = false;
    try {
       Monitor.Enter(myLock, ref haveLock);
    } finally {
       if(haveLock) Monitor.Exit(myLock);
    }
} 

Solution 4

"A lock statement is compiled to a call to Monitor.Enter, and then a try…finally block. In the finally block, Monitor.Exit is called.

The JIT code generation for both x86 and x64 ensures that a thread abort cannot occur between a Monitor.Enter call and a try block that immediately follows it."

Taken from: This site

Solution 5

Just to add a little to Marc's excellent answer.

Situations like this are the very reason for the existence of the lock keyword. It helps developers make sure the lock is released in the finally block.

If you're forced to use Monitor.Enter/Exit e.g. to support a timeout, you must make sure to place the call to Monitor.Exit in the finally block to ensure proper release of the lock in case of an exception.

View more solutions

Comments

[Faraj Khalid]

I am aware of the tryparse, but it is not truly relevant to my question. This was simple code to explain the question - not a true concern regarding the parse. Please replace the parse with any code that will force the catch and makes you comfortable.

[Marc Gravell]

How about throw new Exception("for illustrative purposes"); ;-p

[ShockwaveNN]

Except if a TheadAbortException occurs between the Monitor.Enter and try: blogs.msdn.com/ericlippert/archive/2009/03/06/…

[Marc Gravell]

Actually, no code is "guaranteed" to execute (you could pull the power cable, for example), and that is not quite what a lock looks like in 4.0 - see here

[Ry-]

@MarcGravell: I thought about putting two footnotes about those exact two points. And then I figured it wouldn't matter much :)

[Vort3x]

@MarcGravel: I think everyone assumes always one is not talking about a 'pull the plug' situation, as it is not something a programmer has any control over :)

[CodesInChaos]

That issue is pretty orthogonal to locking IMO. If you get an expected exception, you want to clean up everything, including locks. And if you get an unexpected exception, you have a problem, with or without locks.

[CodesInChaos]

There is at least one case where this isn't true: Thread abortion in debug mode on .net versions before 4. The reason is that the C# compiler inserts a NOP between the Monitor.Enter and the try, so that the "immediately follows" condition of the JIT is violated.

[Gerasimos R]

I think that the situation described above is a generalism. Sometimes exceptions describe catastrophic events. Sometimes they do not. Each one uses them differently in the code. It is perfectly valid that an exception is a signal of an exceptional but non catastrophic event - assuming exceptions=catastrophic, process terminating case is too specific. The fact that is might be a catastrophic event does not remove from the validity of the question - the same train of thought could lead you to never handle any exception, in which case the process will be exited...

[Eric Lippert]

@GerasimosR: Indeed. Two points to emphasize. First, exceptions should be assumed to be catastrophic until determined to be benign. Second, if you are getting a benign exception thrown out of a locked region then the locked region is probably badly designed; it is probably doing too much work inside the lock.

[supercat]

I would posit that the proper thing to do in your scenario would be to have the code that stack-walks out of the bathroom after the bomb goes off put an "out of order" sign there. Simply leaving the bathroom locked might be "safe", but would be needlessly unhelpful. It may make sense to shut everything down if there's anyone that needs to use the bathroom and can't cope with its being unavailable, but if everyone who was in line for the bathroom has a strategy to deal with its being unavailable [perhaps go to another one] there's no reason life shouldn't go on.

[Phil Cooper]

"fatal catastrophic unrecoverable exceptions" like crossing the streams.

[Nicholas Petersen]

All due respect, but it sounds like you are saying Mr. Lippert that the C# design of the lock block which under the hood employs a finally statement is a bad thing. Instead, we should completely crash every app that ever has an exception in a lock statement that wasn't caught within the lock. Maybe that's not what you are saying, but if so, I am really happy the team went the route they did and not your (extremist for purists reasons!) route. All due respect.

[Eric Lippert]

@NicholasPetersen: You are staking out a position that continuing to run a program with potentially corrupted internal data structures is better than either crashing or hanging it. Does your position include silently recovering from null pointer dereferences? Because right now we "completely crash every app that ever had a null pointer dereference". Are you unhappy that C# crashes your app on a null pointer dereference for the "purist" reason that such an app is too buggy to live?

[Eric Lippert]

@NicholasPetersen: Your precis of my position is correct; I do wish that given monitor-based locking and exception throwing are both in the language, that an unhandled exception in a lock ought to be every bit as fatal as a null dereference, and it is a shortcoming to not have this. But the "given" there is important. My larger position is monitor based locking and exception throwing ought to have never both been put in the same runtime in the first place. Monitors are not the best solution to the problem of mutating shared state in a system with multiple threads of control.

[Nicholas Petersen]

@EricLippert thanks for your responses. I respect you low-level guys who have made .NET what it is. Your low-level knowledge means you guys carry burdens of understanding that the rest often don't. Yet I feel what you are arguing would make locks a feared and loathed thing in C#. That then would have an ugly impact on the usability of this beautiful language. I think the solution is let the user themselves bear some responsibility of how this works. In critical cases where the lock encapsulates very bad potentials, then let them put a try catch within the lock themselves.

[Nicholas Petersen]

A thought comes to mind, what if a catch block could follow the lock, so instead of try-catch it would be lock-catch (internally of course it still is just a try with the Monitor): lock(obj) { } catch(Exception ex) { ... } And the catch then would still be before the finally (where the lock is lost). At least it makes some issues explicit, the user would then be able to handle the fact the lock is about to be lost. Just thinking out loud, I don't know.

[Eric Lippert]

@NicholasPetersen: First off, yes, I do fear and loathe locks! :-) Programming is the act of composing correct solutions to small problems into correct solutions to large problems but code containing locks is incomposable. Locks actively work against the very feature of the language that make it usable! Now, that said, if we are going to have locks and exceptions in the same language, and if the lock statement is a sugar for a try-finally, then yes, I very much like your idea of making a catch block. Nice idea!

[Eric Lippert]

In case it is not clear what I mean by non-composable: Suppose we have a method "transfer" that takes two lists, s and d, locks s, locks d, removes an item from s, adds the item to d, unlocks d, unlocks s. The method is only correct if no one ever attempts to transfer from list X to list Y at the same time as someone else attempts to transfer from Y to X. The correctness of the transfer method does not allow you to build a correct solution to a larger problem from it, because locks are unsafe mutations of global state. To safely "transfer" you must know about every lock in the program.

[Nicholas Petersen]

Thanks for your input Eric. I'm glad you liked one of my ideas. One thing I appreciate better now is that you would not have wanted this feature at all. I despise things that haphazardly, in the true sense of the word, terminate the app process (this also happens I think with unhandled exceptions in Tasks, I'm fuzzy on the details, and I believe there's a work around now). But what I can appreciate is that you weren't so much happy about terminating the process, but rather that you didn't like this feature in the first place, in which case there would be no need to terminate the process.