Serious Threading.Monitor issues in .NET 2.0

[Michael Kennedy]

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, just email me and I'll zip up the solution and send it
your way.

Thanks in advance,
Michael

Michael Kennedy

[KJ]

the problem is locking on "this", don't do it. lock on a private object
instead. then see what happens

[Michael Kennedy]

Hi KJ,

That is not the problem. It is a best practice to not do this, but it
should not fail .NET's Monitor class. Thanks for the advice though.

Michael

ps - Once classes "other object" is another classes this.

[KJ]

Could you post all the code (enough to repro)?

[Michael Kennedy]

Hi KJ,

It's kind of big, across 4 files. Can I email it to you?

Thanks,
Michael

[Nicholas Paldino [.NET/C# MVP]]

I would post it, so others can look at it as well. Just zip it up.
--
- Nicholas Paldino [.NET/C# MVP]
- mv*@spam.guard.caspershouse.com

"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11*********************@g44g2000cwa.googlegro ups.com...

Hi KJ,

It's kind of big, across 4 files. Can I email it to you?

Thanks,
Michael

[Michael Kennedy]

Hi,

Ok, just a second. I'll send it up for you guys.

Michael

[Michael Kennedy]

Hi Guys,

You can download the solution here:

http://www.unitedbinary.com/TEMP-001...8/LockTest.zip

Let me know what you think,
Michael

[Michael Kennedy]

Hi KJ,

In case you didn't see the reponse to Nicholas...

You can download the solution here:

http://www.unitedbinary.com/TEMP-001...8/LockTest.zip

Let me know what you think,
Michael

[KJ]

When you wrote "solution", I thought you meant solution to the problem,
not VS .sln.

I will run this at home later, since I don't have vs2005 on this
machine.

Sorry if my first answer was on the glib side.

[Michael Kennedy]

Hi KJ,

Thanks. Let me know what you think. I could be missing something, but I
think the code is correct, and yet it's kickin' exceptions out left and
right.

If you want to see what it's output is like without VS.NET 2005, you
could look at the SampleOutput.txt file including in one of the
projects.

Take care,
Michael

[Willy Denoyette [MVP]]

You should never call Thread.Abort asynchronously, that is from another
thread than the owning thread, like you do.

When your thread gets aborted when Monitor.Enter(this) (in DoItSafeLock())
is waiting to aquire the lock, the wait will be abandoned and the finally
clause ( so Monitor.Exit( this );) executed, resulting in a
SynchronizationLockException

Willy.

"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11**********************@z14g2000cwz.googlegr oups.com...

Hi KJ,

In case you didn't see the reponse to Nicholas...

You can download the solution here:

http://www.unitedbinary.com/TEMP-001...8/LockTest.zip

Let me know what you think,
Michael

[Michael Kennedy]

Hi Willy,

That is a valid point. ASP.NET throws thread abort excpetions. Is this
always from the calling thread in ASP.NET?

How can I ensure that a client of my libraries won't do that and
subsequently permanently lock my objects?

Thanks,
Michael

[Willy Denoyette [MVP]]

"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11**********************@z14g2000cwz.googlegr oups.com...

Hi Willy,

That is a valid point. ASP.NET throws thread abort excpetions. Is this
always from the calling thread in ASP.NET?

How can I ensure that a client of my libraries won't do that and
subsequently permanently lock my objects?

Thanks,
Michael

ASP.NET only aborts threads as a result of Apllication Domain unloads, that
means that the domain and it's loaded code and objects will be removed
anyway.
Clients of your library code should not abort threads they don't own, this
is true for unmanaged code as well as for managed code.
Note that V2 of the framework contains some reliability features to protect
you from issues resulting from asynchronous thread abort's (proper clean-up
of unmanaged resources and locks), but again this doesn't mean it's safe to
introduce asynchronous thread aborts while continuing to execute code in
that AD.
Please read my reply in the clr NG where you posted the same question.

Willy.

[Michael Kennedy]

Willy,

That is not true. Response.Redirect(url) or Response.Redirect(url,
true) will also throw thread abort exceptions.

Regards,
Michael

[Michael Kennedy]

Willy,

One more thing:

Clients of your library code should not abort threads they don't own, this
is true for unmanaged code as well as for managed code.

True, but consider this.

1. Client creates a thread
2. Clent creates an object from my library and interacts with it on
their thread.
3. My objects uses lock properly.
4. Client aborts *their* thread.
5. My object is dead-locked.

Regards,
Michael

[Willy Denoyette [MVP]]

"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11*********************@o13g2000cwo.googlegro ups.com...

Willy,

One more thing:

Clients of your library code should not abort threads they don't own,
this
is true for unmanaged code as well as for managed code.

True, but consider this.

1. Client creates a thread
2. Clent creates an object from my library and interacts with it on
their thread.
3. My objects uses lock properly.
4. Client aborts *their* thread.
5. My object is dead-locked.

Regards,
Michael

I said you can only safely call Thread.Abort in the code executing on that
thread (synchronously). A thread started by a client, does not mean the
client owns the thread, it's the thread procedure that owns the thread it's
running in.
That means calling Thread.CurrentThread.Abort() is safe, anything else is
unsafe and should only result from a AD unload.

Willy.

[Willy Denoyette [MVP]]

"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11**********************@g47g2000cwa.googlegr oups.com...

Willy,

That is not true. Response.Redirect(url) or Response.Redirect(url,
true) will also throw thread abort exceptions.

Regards,
Michael

Sure, but, Response.Redirect(url) and Response.Redirect(url), do throw a
ThreadAbortException they don't call Thread.Abort().
That means that the CURRENT thread (the one that throws) will terminate as
ThreadAbortException cannot be catch'd, this is totally different from
calling SomeOtherThread.Abort() on another thread, which injects a
ThreadAbortException in "SomeOtherThread" thread.

Willly.

[KJ]

Hmm. Let me see if I have your concern correct: A user of your class
may forcibly abort threads instantiated and running methods of that
class? Perhaps you might take another tact: simply make the threading
aspects of the class "opaque" to clients of it. This is possible using
a "design pattern" such as Strategy, or Producer/Controller, or
whatever you please. Whether or not this is a .net bug is something I
wouldn't worry much about. You could actually catch the
SynchronizationLockException in the finally block, no?

If possible, don't give clients of your code the opportunity to
suspend/start/stop/abort (etc.) your threads, or lock your classes'
objects. What a client doesn't know can't hurt them. If they call
YourObject.Foo(), and that starts a new thread running the Bar()
method, so be it. The caller doesn't need to know this.

Hope this helps, even if it's not 100% correct in every detail.

[Marc Gravell]

Isn't this simply a minor code issue? Avoiding the "this" /
"some-other-object" debate, shouldn't this be:

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

Done the original way, surely you don't know (in the finally block where you
release the lock) that you have actually taken the lock; with the above code
you know that if you made it to the try block you *have* the lock...

Apologies if this is my naivety showing through and the above is in fact
incorrect...

Marc
"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11*********************@f14g2000cwb.googlegro ups.com...

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, just email me and I'll zip up the solution and send it
your way.

Thanks in advance,
Michael

Michael Kennedy

[Willy Denoyette [MVP]]

When your thread gets aborted after you have obtained the lock but before
you entered the try block, you end with an unreleased lock. The point here
is not the sequence by which things are done, it's simply that you should
not call Abort() on another running thread (called an asynchronous thread
abort).

Willy.
"Marc Gravell" <mg******@rm.com> wrote in message
news:u7**************@tk2msftngp13.phx.gbl...

Isn't this simply a minor code issue? Avoiding the "this" /
"some-other-object" debate, shouldn't this be:

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

Done the original way, surely you don't know (in the finally block where
you release the lock) that you have actually taken the lock; with the
above code you know that if you made it to the try block you *have* the
lock...

Apologies if this is my naivety showing through and the above is in fact
incorrect...

Marc
"Michael Kennedy" <mk******@unitedbinary.com> wrote in message
news:11*********************@f14g2000cwb.googlegro ups.com...

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, just email me and I'll zip up the solution and send it
your way.

Thanks in advance,
Michael

Michael Kennedy

[Jon Skeet [C# MVP]]

Michael Kennedy wrote:

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.

Actually, in .NET 1.1 Monitor.Exit is completely broken - the docs
claim it throws SynchronizationLockException if the current thread
doesn't own the lock, but in practice it doesn't.

What you're seeing is a consequence of Monitor.Exit doing the *right*
thing in 2.0, combined with the bad practice of aborting (or
interrupting - that can have the same effect) another thread.

See http://www.pobox.com/~skeet/csharp/threads/abort.shtml for more
information.

Jon

[Michael Kennedy]

Hi Willy,

I think you've convinced me to stop worrying.

My main concert is just that someone would consume my object in their
thread, then abort their thread and effectively break my threading
logic. But after talking with you guys I've seen basically you're
screwed either way entery/try or try/enter.

So I'll just hope no one does that and they get what they get if they
do. ;)

Thanks,
Michael

[Michael Kennedy]

Hi Jon,

What you're seeing is a consequence of Monitor.Exit doing the *right*
thing in 2.0, combined with the bad practice of aborting (or
interrupting - that can have the same effect) another thread.

Now that I've thought about this for awhile, I see that tou're right.
Thanks for the info!

Regards,
Michael

[Michael Kennedy]

Hi KJ,

I am familiar with design patterns, but I don't know of anything I
could do to avoid someone creating their own thread, using my objects
(which may not even use a thread directly), aborting their thread, and
breaking my usage of lock().

See my response to Willy for a little more on that:

http://groups.google.com/group/micro...1decd659226df1

Thanks for the help!
Michael

[Michael Kennedy]

Hi Marc,

Yeah, that part isn't really the issue (this vs lockObj). As Willy has
convinced me, you're basically helpless to keep things working right
once you go down the path to Thread.Abort(). try/enter/... or
enter/try/... either way results in unbalanced enter/exit calls.

Thanks!
Michael