locking a resource via lock within try. Is it wrong?

Is there anything wrong with using lock with a try block? I remember reading somewhere that we should always try to put minimum amount of code within try block and lock itself int开发者_如何学JAVAernally uses a try-finally block, do you guys see something wrong here.I need to deal with the fact that the code within that lock block can throw exception

try  
{  
   lock(syncblk)  
   {  
        // do some processing  
    }  

}  
catch(Exception e)  
{  
    // do something with exception  
}  

I need to deal with the fact that the code within that lock block can throw exception

And there's your problem. That's a terrible situation to be in.

Why are you locking in the first place? Usually the reason why you lock something is because you want to implement the following logic:

• lock the door
• make a mess
• clean it up
• unlock the door

If you do that, then no one who honours the locked door ever sees the mess.

For example, you might want to swap values of variables "left" and "right" in a threadsafe manner, so you:

• take the lock
• read the left variable into tempLeft
• read the right variable into tempRight
• write tempLeft into right
• we just made a mess; the original value of 'right' has gone missing
• write tempRight into left
• we've cleaned up the mess, all is well with the world again
• release the lock

Now suppose an exception is thrown after the mess is made. What happens? We jump straight to the unlock, leaving the mess for another thread to see.

That's why you should never throw an exception inside a lock; it completely defeats the purpose of the lock! The whole point of a lock is to ensure that state is always observed to be consistent by all threads except the one responsible for cleaning up the mess.

If you have an exception that can be thrown from inside a lock, the best thing to do is to get out of that horrible situation. If you can't do that, then make sure that you can either (1) destroy the process utterly as soon as the exception escapes the lock, so that the mess you made cannot cause data loss or other harm -- do a FailFast and nuke the process from orbit, it's the only way to be sure -- or (2) write rollback code that undoes whatever operation you were attempting before the lock is exited; that is, clean up the mess back to the original state.

If the latter is your strategy then don't put the try block outside the lock; it's useless there because the instant control leaves the lock via the exception another thread can be crashing and dying because of the mess you left exposed to it. Put the try that deals with the exception inside the lock:

lock(whatever)
{
    try
    {
        MakeAMess();
    }
    finally
    {
        CleanItUp();
        // Either by completing the operation or rolling it back 
        // to the pre-mess state
    }
}

If you have strong reliability requirements then dealing with locked critical sections which can throw exceptions is an extremely difficult programming task best left to experts; you might consider using a constrained execution region if you find yourself in this situation a lot.

I think you can do it your way but here is the MSDN description on lock for your information. Please refer to http://msdn.microsoft.com/en-us/library/ms173179.aspx for more info.

Using the lock (C#) or SyncLock (Visual Basic) keyword is generally preferred over using the Monitor class directly, both because lock or SyncLock is more concise, and because lock or SyncLock insures that the underlying monitor is released, even if the protected code throws an exception. This is accomplished with the finally keyword, which executes its associated code block regardless of whether an exception is thrown.

So I am not sure what kind of exception you are referring to but if you concern is that you may not be able to release the lock because of exception, you do not have to worry about it.

you can always use the longer syntax like this:

System.Threading.Monitor.Enter(x);
try {
   ...
}
catch(Exception e)
{
}
finally {
   System.Threading.Monitor.Exit(x);
}