Serious Threading.Monitor issues in .NET 2.0

0 views
Skip to first unread message

Michael Kennedy

unread,
Nov 30, 2005, 12:43:43 PM11/30/05
to
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

unread,
Nov 30, 2005, 2:03:58 PM11/30/05
to
the problem is locking on "this", don't do it. lock on a private object
instead. then see what happens

Michael Kennedy

unread,
Nov 30, 2005, 2:06:26 PM11/30/05
to
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

unread,
Nov 30, 2005, 2:23:52 PM11/30/05
to
Could you post all the code (enough to repro)?

Michael Kennedy

unread,
Nov 30, 2005, 2:27:45 PM11/30/05
to
Hi KJ,

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

Thanks,
Michael

Nicholas Paldino [.NET/C# MVP]

unread,
Nov 30, 2005, 2:30:22 PM11/30/05
to
I would post it, so others can look at it as well. Just zip it up.


--
- Nicholas Paldino [.NET/C# MVP]
- m...@spam.guard.caspershouse.com

"Michael Kennedy" <mken...@unitedbinary.com> wrote in message
news:1133378865....@g44g2000cwa.googlegroups.com...

Michael Kennedy

unread,
Nov 30, 2005, 2:32:49 PM11/30/05
to
Hi,

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

Michael

Michael Kennedy

unread,
Nov 30, 2005, 2:47:46 PM11/30/05
to
Hi Guys,

You can download the solution here:

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

Let me know what you think,
Michael

Michael Kennedy

unread,
Nov 30, 2005, 2:49:21 PM11/30/05
to
Hi KJ,

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

KJ

unread,
Nov 30, 2005, 4:53:07 PM11/30/05
to
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

unread,
Nov 30, 2005, 4:56:18 PM11/30/05
to
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]

unread,
Nov 30, 2005, 5:28:24 PM11/30/05
to
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" <mken...@unitedbinary.com> wrote in message

news:1133380161.5...@z14g2000cwz.googlegroups.com...

Michael Kennedy

unread,
Nov 30, 2005, 5:34:18 PM11/30/05
to
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]

unread,
Nov 30, 2005, 6:04:23 PM11/30/05
to

"Michael Kennedy" <mken...@unitedbinary.com> wrote in message
news:1133390058.4...@z14g2000cwz.googlegroups.com...

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

unread,
Nov 30, 2005, 6:09:19 PM11/30/05
to
Willy,

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

Regards,
Michael

Michael Kennedy

unread,
Nov 30, 2005, 6:11:08 PM11/30/05
to
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]

unread,
Nov 30, 2005, 6:52:09 PM11/30/05
to

"Michael Kennedy" <mken...@unitedbinary.com> wrote in message
news:1133392268....@o13g2000cwo.googlegroups.com...

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]

unread,
Nov 30, 2005, 7:03:34 PM11/30/05
to

"Michael Kennedy" <mken...@unitedbinary.com> wrote in message
news:1133392159.0...@g47g2000cwa.googlegroups.com...

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

unread,
Dec 1, 2005, 12:02:41 AM12/1/05
to
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

unread,
Dec 1, 2005, 4:25:56 AM12/1/05
to
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" <mken...@unitedbinary.com> wrote in message

news:1133372623....@f14g2000cwb.googlegroups.com...

Willy Denoyette [MVP]

unread,
Dec 1, 2005, 4:47:01 AM12/1/05
to
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" <mgra...@rm.com> wrote in message
news:u7sL9kl9...@tk2msftngp13.phx.gbl...

Jon Skeet [C# MVP]

unread,
Dec 1, 2005, 7:48:19 AM12/1/05
to
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

unread,
Dec 1, 2005, 12:03:33 PM12/1/05
to
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

unread,
Dec 1, 2005, 12:05:56 PM12/1/05
to
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

unread,
Dec 1, 2005, 12:09:17 PM12/1/05
to
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/microsoft.public.dotnet.languages.csharp/tree/browse_frm/thread/1cbd7676fc15794c/15e976f6b4b57114?rnum=11&hl=en&_done=%2Fgroup%2Fmicrosoft.public.dotnet.languages.csharp%2Fbrowse_frm%2Fthread%2F1cbd7676fc15794c%2F545b9035f5be184d%3Flnk%3Darm%26hl%3Den%26#doc_4f1decd659226df1

Thanks for the help!
Michael

Michael Kennedy

unread,
Dec 1, 2005, 12:12:40 PM12/1/05
to
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

Reply all
Reply to author
Forward
0 new messages