Serious Threading.Monitor issues in .NET 2.0

Thread view:

Enable EMail Alerts

Start New Thread

Thread rating:

Michael Kennedy - 30 Nov 2005 21:32 GMT

Hi,

I have been looking into the claim that the keyword lock is not safe
when exceptions are possible. That lead me to try the following code,
which I think has uncovered a serious error in the .NET 2.0 framework.
Note that this runs better, but not perfectly, on .NET 1.1.

Note: The numbers are line numbers needed to match the callstack of the
exception below.

// Class WorkItem ...
34 public void DoItSafeLock()
35 {
36 try
37 {
38 Monitor.Enter(this);
39 Thread.Sleep(10);
40 }
41 finally
42 {
43 Monitor.Exit(this); // <-- SynchronizationLockException!
44 }
45 }

Here's what I get when I run this code on a different thread than the
one that created the object holding this method (WorkItem class):

System.Threading.SynchronizationLockException: Object synchronization
method was called from an unsynchronized block of code.
at WorkItems.WorkItem.DoItSafeLock() in
D:\LockTest\WorkItems\WorkItem.cs:line 43
at WorkItems.Worker.ExecuteWorkItem() in
D:\LockTest\WorkItems\Worker.cs:line 127

Notice that the error is that on line 43, the object does not hold a
lock on itself (this). How is this possible given that it made it
through the Monitor.Enter(this) call on line 38?!?

Does any one know what's going on here? How can the monitor be entered
and yet no lock is acquired?

I have a sample application demonstrating the error if any one wants to
see the code, it can be download here:

http://www.unitedbinary.com/TEMP-001-2005-05-28/LockTest.zip

Thanks in advance,
Michael

Michael Kennedy

PS - This has been reposted to these newsgroups. It was originally
posted to the C# newsgroup only.

Reply to this Message

Jon Shemitz - 30 Nov 2005 21:59 GMT

> // Class WorkItem ...
> 34 public void DoItSafeLock()
[quoted text clipped - 23 lines]
> lock on itself (this). How is this possible given that it made it
> through the Monitor.Enter(this) call on line 38?!?

Note that the error message is that Exit "was called from an
unsynchronized block of code" - NOT that you do not have a lock on the
`this` reference. Note also that `lock (this) statement;` expands to

Monitor.Enter(this);
try {statement;}
finally {Monitor.Exit(this);}

NOT to

try {Monitor.Enter(this); statement;}
finally {Monitor.Exit(this);}

The difference is that the try statement in the three line version
(the lock expansion) is a synchronized block, while the try statement
in the two line statement is not. (The two line version will call Exit
even if Enter fails; the three line version will not.) I downloaded
your sample app and got the same exception (with 2.0, beta 2); when I
changed your WorkItem.DoItSafeLock to

public void DoItSafeLock()
{
Monitor.Enter(this);
try
    {
//Monitor.Enter( this );
        Thread.Sleep( 10 );
    }
    finally
    {
        Monitor.Exit( this );
    }
}

I did not get an exception.

Signature

<http://www.midnightbeach.com>

Reply to this Message

Michael Kennedy - 30 Nov 2005 22:09 GMT

Hi Jon,

Exactly, the "safeness" of it was to try to fix the inherent faults in
how lock behaves.

If there is an exception, it might leave this permanently locked.
Consider how lock expands:

lock(this)
{
// Do stuff
}

becomes:

Monitor.Enter(this);
// <-- Exception(e.g. threadabort), exit is never called after enter.
try
{
// Do stuff
}
finally
{
Monitor.Exit(this);
}

Notice that the program proves that lock() is unsafe. I was trying to
show that the try/enter/finally/leave method was in fact safe. But
instead I found the error that you also found.

Do you agree that it should not be an error?

Thanks,
Michael

Reply to this Message

Jon Shemitz - 30 Nov 2005 22:29 GMT

> Exactly, the "safeness" of it was to try to fix the inherent faults in
> how lock behaves.

Imho, `lock` is fine. In general, you want to enter a state outside of
the try/finally block that exits the state, precisely so that you
don't try to undo what has never been done. Consider, for example,
locking `SomeRef` when SomeRef is null: You do NOT want to try to
acquire the monitor within the try block and release the monitor
within the finally block!

> Monitor.Enter(this);
> // <-- Exception(e.g. threadabort), exit is never called after enter.
[quoted text clipped - 6 lines]
> Monitor.Exit(this);
> }

Yes, this is why thread abort is no safer in managed code that in
unmanaged code.

> Notice that the program proves that lock() is unsafe. I was trying to
> show that the try/enter/finally/leave method was in fact safe. But
> instead I found the error that you also found.
>
> Do you agree that it should not be an error?

No, I think you found a PEBCAC error. The error message you got seems
exactly right.

Signature

<http://www.midnightbeach.com>

Reply to this Message

Michael Kennedy - 30 Nov 2005 22:45 GMT

I suppose there is no way out of this eh?

The try/enter/finally/leave could prematurely unlock the code if you
have "doublly locked it"

try
{
<-- error here is a double leave
enter
try { enter }
finally {leave}
}
finally {leave}

and

// lock version
enter
<-- error here is a permently locked.
try
{
enter
try { }
finally {leave}
}
finally {leave}

Does ASP.NET only send thread abort exceptions from the main thread?

Thanks,
Michael

Reply to this Message

Michael Kennedy - 30 Nov 2005 22:30 GMT

Hi Jon,

[Sorry if this is a somewhat duplicated I think it might have not
posted correctly, so I'll try again]

Jon, I think I misunderstood you. You did not get an exception?

Did you look at the usage of lock() in the program? It proves that

lock(obj)

which is

Monitor.Enter(obj)
try {}
finally {Monitor.Leave(obj)}

Is definitely not safe. So if both try/enter/finally/leave and
enter/try/finally/leave is not safe, what do you suggest we use for
thread synch? Those are basically the two ways you could do it and this
program shows they are BOTH broken.

Thanks,
Michael

Reply to this Message

Willy Denoyette [MVP] - 30 Nov 2005 22:50 GMT

> Hi Jon,
>
[quoted text clipped - 20 lines]
> Thanks,
> Michael

As I replied in the csharp NG (pease don't multipost for that reason), what
is unsafe, is calling Thread.Abort from another thread (asynchronous thread
aborts are a NO NO in .NET), if you do call Thread.Abort to terminate
another running thread, you should regard your Application Domain as doomed
and you should unload it.

Note however, that "lock" as been made more reliable in the face of Thread
Aborts resulting from AD unloads in V2 of the FW.
In this version, the JIT (actually a hack) doesn't condider the window
between the lock aquisition and entering the protected block. That means
that the lock will only be released when effectively aquired. But, that
doesn't mean you can call thread abort from user code and continue to
execute in the doomed AD.

Willy.

Reply to this Message

Michael Kennedy - 30 Nov 2005 23:03 GMT

Jon,

Sorry I missed this point:

>The two line version will call Exit
> even if Enter fails; the three line version will not.

When will Monitor.Enter fail?

Also, you may need to run it several times to get the second part to
fail. Basically your rewrite (via using lock(this)). You will get
errors, they are just rare race conditions. See the SampleOutput.txt
file in the project.

Finally, if try/enter/finally/leave is not acceptible and lock() {}
clearly is not acceptible, then what other options do we have for
synchronization? Neither work. What can we do?

Thanks,
Michael

Reply to this Message

Jon Shemitz - 01 Dec 2005 02:05 GMT

> When will Monitor.Enter fail?

When you passs it null. Or, perhaps, in the face of an out-of-memory
situation (if lock() needs to create a new synch block, and can't).

> Finally, if try/enter/finally/leave is not acceptible and lock() {}
> clearly is not acceptible, then what other options do we have for
> synchronization? Neither work. What can we do?

Only call Abort() when you're aborting the whole app.

Signature

<http://www.midnightbeach.com>

Reply to this Message

Serious Threading.Monitor issues in .NET 2.0

Free Magazines