Anonymous Delegates, captured variables and ThreadStart

[ktrvnbq02]

Hi,

I recently came to debug some old code that was causing a
StackOverflowException. The code in question makes significant use of
recursion and with large data structures it exhausted the stack. As
the code is complicated and would take significant re-testing it
wasn't practical to re-write it at present.

As a work-around until the code could be re-written, I decided to
invoke it on a new thread to which I had allocated a larger stack, and
then block the calling thread until the work was complete. The code I
wrote was like follows:

----8<----

MyResult[] results;
Exception ex;
ThreadStart ts = new ThreadStart(
delegate
{
try
{
results = DoWork();
}
catch (Exception threadEx)
{
ex = threadEx;
}
}
);

Thread t = new Thread(ts, STACKSIZE);
t.Start();
t.Join();

if (ex != null)
{
// etc...
}

foreach (MyResult result in results)
{
// etc...
}

----8<----

This fixed the StackOverflowException and appeared to work. A short
while later it occurred to me that this approach was not thread-safe.

It's easy to look at that code and read it as if you're only using
local variables, but in fact you are using a public class member
variable in a class generated by the compiler for the anonymous
delegate. Whilst there's nothing to synchronise as such because the
main thread is blocked waiting, you *do* need a memory barrier to be
thread-safe.

To prove my theory I constructed the following test case. On my
machine, with a Release build and no debugger attached, the while loop
never exits as the hasFinished variable gets enregistered and so never
sees the update from the other thread.

----8<----

using System;
using System.Threading;

namespace ThreadObserver
{
class Program
{
static void Main(string[] args)
{
bool hasFinished = false;

ThreadStart ts = new ThreadStart(
delegate
{
Thread.Sleep(5 * 1000);
hasFinished = true;
}
);

Thread t = new Thread(ts);
t.Start();

while (!hasFinished)
{
}

Console.WriteLine("Finished.");
Console.Read();
}
}
}

----8<----

It's actually fairly easy to fix, even still using anonymous delegates
- just make sure you capture a lock object to use when reading/writing
the return value(s). The following code always exits successfully:

----8<----

using System;
using System.Threading;

namespace ThreadObserver
{
class Program
{
static void Main(string[] args)
{
object myLock = new object();
bool hasFinished = false;

ThreadStart ts = new ThreadStart(
delegate
{
Thread.Sleep(5 * 1000);
lock (myLock)
{
hasFinished = true;
}
}
);

Thread t = new Thread(ts);
t.Start();

bool fin;
do
{
lock (myLock)
{
fin = hasFinished;
}
} while (!fin);

Console.WriteLine("Finished.");
Console.Read();
}
}
}

----8<----

This is probably an unusual case, but thought this would be worth
posting in case anyone else has used a similar pattern or is thinking
of doing so.

It highlighed to me just how necessary it is to keep remembering what
the compiler is and isn't doing for you when you use convenient
language features such as anonymous delegates.



Regards,

Matt

 

[Jon Skeet [C# MVP]]

Hi,

I recently came to debug some old code that was causing a
StackOverflowException. The code in question makes significant use of
recursion and with large data structures it exhausted the stack. As
the code is complicated and would take significant re-testing it
wasn't practical to re-write it at present.

As a work-around until the code could be re-written, I decided to
invoke it on a new thread to which I had allocated a larger stack, and
then block the calling thread until the work was complete. The code I
wrote was like follows:

----8<----

MyResult[] results;
Exception ex;
ThreadStart ts = new ThreadStart(
delegate
{
try
{
results = DoWork();
}
catch (Exception threadEx)
{
ex = threadEx;
}
}
);

Thread t = new Thread(ts, STACKSIZE);
t.Start();
t.Join();

if (ex != null)
{
// etc...

}

foreach (MyResult result in results)
{
// etc...

}

----8<----

This fixed the StackOverflowException and appeared to work. A short
while later it occurred to me that this approach was not thread-safe.

It's easy to look at that code and read it as if you're only using
local variables, but in fact you are using a public class member
variable in a class generated by the compiler for the anonymous
delegate. Whilst there's nothing to synchronise as such because the
main thread is blocked waiting, you *do* need a memory barrier to be
thread-safe.

I'm not at all sure you do, because of your call to Join().

I can't possibly see how that would work without a memory barrier, and
the writes have to happen before the previous thread terminates so
there's an effective memory barrier there.

To prove my theory I constructed the following test case.

That code doesn't prove that your original code was problematic - your
test case doesn't call Thread.Join. Show me a case where you call
Join() but *then* don't see the most recently written value and I'll
admit you've got a problem.

I can't point to any documentation in Join() that proves there's
effectively a memory barrier in the calling thread after the target
thread has exited, but I would be very, very surprised if this were
not the case.

I can mail Joe Duffy and see if he has a definitive answer though, if
you'd like.

Jon