5

Can Monitor.Enter throw any exception. I am doing a code review and find that Monitor.Enter is before try block. Do you see any issues with in? 

Monitor.Enter(...)
try
{
    ...
}
finally
{
    Monitor.Exit(..)
}
BartoszKP
  • 34,786
  • 15
  • 102
  • 130
palm snow
  • 2,392
  • 4
  • 29
  • 49
  • 3
    Having Enter() inside the try block is a bug. You'll call Exit() when it could have failed to Enter(). You'd only have to bellyache over an exception raised *after* the Enter() call, *before* entering the try block. Which was in fact a bug in the x64 jitter: http://www.bluebytesoftware.com/blog/2007/01/30/MonitorEnterThreadAbortsAndOrphanedLocks.aspx Also the motivation behind the new 4.0 Monitor.Enter(object, ref bool) overload. – Hans Passant Jan 12 '12 at 18:32

4 Answers4

13

This is the correct pattern, whether Enter() throws (can throw) or not.

Only after the call to Enter() succeeds your code is under the responsibility to call Exit().

Suppose the call to Enter() fails. Then calling the corresponding Exit() is simply incorrect, it will make matters worse. So Enter() has to be outside (before) the try block.

H H
  • 263,252
  • 30
  • 330
  • 514
10

Hans Passant's comment is of course correct. If Monitor.Enter throws before the lock is taken then you do not want the finally to run. If it throws after the lock is taken and after the try is entered, then the lock is released. (More on this later.) But if the throw happens after the lock is taken but before the try is entered, then the lock will never be cleaned up.

This is a rare but possible situation.

In C# 4 we changed the codegen of the lock statement so that the monitor enter is inside the try. This ensures that the lock is always released if something throws after the lock is taken. However, note that this might still be wrong. If something throws after the lock is taken then whatever non-atomic mutation the lock is protecting might be half-completed, and the finally block then unlocks the lock and allows access to the inconsistent state! The fundamental problem here is that you shouldn't be throwing inside a lock in the first place.

See my article about the issue for more discussion.

BartoszKP
  • 34,786
  • 15
  • 102
  • 130
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • I'd love to get a more general mechanism that works on custom `Enter/Leave` pairs, too. The current `using` statement, which is often used for this suffers from the same problem as the old lock statement. – CodesInChaos Jan 13 '12 at 10:36
  • @CodeInChaos: But the difference is that the worst that should happen if a throw happens in the wrong place and an allocated unmanaged resource is not cleaned up in a timely manner is someone doesn't get to use that resource. A lock that is cleaned up when the state of the locked object is inconsistent is a correctness problem, not a politeness problem. – Eric Lippert Jan 13 '12 at 14:36
  • A general, safe `Enter/Leave` pattern would have applications beyond simply freeing unsafe resources. I've seen `using` statements for custom locking, database transactions,... Custom locking would benefit a lot from such a feature, making it finally both safe and convenient. Personally I think that the current version `lock` statement should not exist in the first place, but be expressed using such a more general feature. But it's too late to change that now. – CodesInChaos Jan 13 '12 at 15:14
2

Monitor.Enter can throw at least the following exceptions

  • ArgumentNullException of the parameter is null
  • ThreadInterruptedException if the thread doing the Enter has it's Interrupt method invoked.
JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
0

If it acquires the lock, then no.

But an exception might be thrown between the Monitor.Enter and the try block.

The recommended method is the new Enter method, new in .NET 4:

public static void Enter( obj, ref bool lockTaken )
Nick Butler
  • 24,045
  • 4
  • 49
  • 70