analyzing the correctness of multithreaded behavior is
Tell me about it
For a whale of a good time, try doing this in a kernel mode device driver
Ah - I didn't realise .NET did this. It's an interesting debate though
- I know when I was writing some servlet code in Java, it was helpful
in debug to set the name of the thread itself to the URL of the request
being processed.
Given the .NET behaviour, perhaps I shouldn't be setting the Name
property of the individual threads? Not sure.
Agreed. I feel this is one of those issues that does not have a single
"correct" answer.
Rather than make it a "set once" property (which seems a bit clunky to
me) I think I'd rather have another constructor for the pool which
takes the name as a parameter - and then I can remove the "setter" for
the Name property entirely.
That would work. Another option is to add the name to the workitem, and then
set the name of the thread to that value immediately before invoking the
worker callback. Since the only legitimate use of the thread name is for
diagnositic/instrumentation purposes this seems to me like a good place to
set the name. That way the thread takes on the identity of the worker item
being processed. You could set the name to idle when it has no more items
left to process.
Ooh, nasty. I hadn't thought of that.
Yikes. That involves asking the delegate for the invocation list and
doing each invocation myself, correct?
Yes, but that isn't any more onerous that handling the add/remove as you are
already doing. I think something along the lines of the following would work
(caveat - this is completely untested and off the top of my head).
bool _runningList = false;
object _unsubscribeLock = new object();
ArrayList _unsubscriptionList = new ArrayList();
public event ExceptionHandler WorkerException
{
<snip>
remove
{
lock (eventLock)
{
exceptionHandler -= value;
if ( _runningList )
lock(queueLock)
{
queue.Enqueue(value);
if ( _runningList == true )
{
lock(_unsubscriptionList)
{ // add delegate to temp list
_unsubscriptionList.Add(value);
}
}
}
}
}
}
and in the invocation list
void OnException(Exception e)
{
// NOTE: I disagree with ignoring the exceptions, but that's a different
tale

// If anything goes wrong within the exception handler,
// ignore it.
try
{
ExceptionHandler eh;
lock (eventLock)
{
eh = exceptionHandler;
}
if (eh != null)
{
_unsubscriptionList.Clear(); // reset list
_runningList = true;
foreach ( ExceptionHandler d in eh.GetInvocationList() )
{
if ( !IsUnsubscribed(d) )
d(this,e);
}
//eh(this, e);
}
}
catch // add better exception handling here
{
}
finally
{
_runningList = false;
}
}
private bool IsUnsubscribed(ExceptionHandler d)
{
lock(_unsubscribeLock)
{
foreach ( ExceptionHandler eh in _unsubscriptionList )
{
if ( d.Equals(eh) )
return true;
}
}
return false;
}
I would also change the lock used to synchronize the invocation list from
the event subscription so that the user can unsubscribe while processing the
exception even if called on a different thread. As it is you may get into a
deadlock if the user makes a blocking call to another thread that tries to
unsubscribe from the event while blocking in the worker callback.
Note: This is just a suggestion - this would not work for the final solution
because it does not account for multiple exceptions that may be processed
from multiple worker threads for multiple work items. So it really needs a
queue per worker thread.
Right. I thought I'd documented that as intended behaviour, but I see
that I haven't. (It turns out I've done it in ThreadController.)
I haven't seen that class yet. Perhaps you could copy the documentation over
to this class.
But then, do I definitely want the ThreadPool to stop if a non-
CLSCompliant exception is thrown? ThreadAbortException won't be
swallowed, because it *can't* be swallowed. Then again, I see that I
don't have an extra catch{} block in the main worker loop, so I ought
to at least be consistent.
My take on non-CLSCompliant exceptions is...let the runtime barf if you get
one (IOW, don't catch it). The reason is that as far as I know the only way
you can even throw a non-clscompliant exception is to do it directly in
IL...I don't know of any language that will even let you generate one. You
can't get one of those even from interop because the runtime catches and
translates all exceptions from the interop layer. So if you do get one
you're are probably in an indeterminate state anyway and there are no
guarantees about what will happen next. I'd rather exit the app then keep
running under those conditions.
This is definitely an area I'm relatively unhappy about in .NET. If a
non-compliant exception is thrown, but you want to keep going anyway,
there's not a lot you can do - there isn't an exception to pass to any
handlers.
Agreed. I understand why they designed it in, but the problem is that they
did not give us a complete story about what it means, how it should be
handled, and what to expect if it is not handled.
I take your point about not just swallowing it wholesale - but if I
call SwallowException, what should a library do in this case? It can't
know about debugging/ogging etc, and we're already in an OnException
case here - I don't want to go down a "OnExceptionInExceptionHandler"
route
Advice welcome here.
I feel that it is ok to do at least some minimal tracing - heck, the runtime
itself will generate tracing when a listener is subscribed. I'd rather have
a trace facility built into the threadpool that the user can opt out of at
runtime. These kinds of breadcrumbs are invaluable when troubleshooting
nasty bugs. There could be a static property called UseDebugOutput that by
default is set to true. When it is true it could execute some logic like
this.
static void DebugOutput(string format,params object[] args)
{
if ( UseDebugOutput )
{
string msg = string.Format(format,args);
if ( System.Diagnostics.Debugger.IsAttached )
System.Diagnostics.Debug.WriteLine(msg);
else
System.Diagnostics.Trace.WriteLine(msg);
}
}
Right. That's easy enough to do if I go down the manual invocation
list, but not easy otherwise. I'll think about it.
Fair enough.
I think for the sake of convenience it would be better to make it a ref
parameter which would always have the value "false" on entry, just so
that in the normal case of "keep processing" the handler didn't have to
specifically set the value. What do you think?
That sounds like a good idea.
That's a nice idea. The disadvantage is that the parameters couldn't be
garbage collected until after a job completed or failed. I don't know
how significant a disadvantage that would be.
I don't think that matters much because if an exception occurs it is already
going down a sub-optimal code path anyway.
Not entirely sure what you mean here - could you elaborate?
I was thinking about logging it to the event log itself. There are some
exceptions that are more setup or configuration type errors (setting the
MinThreadPoolSize to an invalid value) that should be obviously apparent,
but some runtime problems are extremely trickly to capture, let alone
diagnose and fix. In the case of a threadpool, because unhandled exceptions
go unnoticed there is a real danger that the application wont even realize
that an exception occurred and it could wind up hanging without having a
clue what went wrong. This could happen if the app did not subscribe to the
exception event. You could even filter the logging so that it only logged if
the exception event was not subscribed to.
That's a nice idea. Again, passing in the work item which is about to
be executed, right?
Yup. Or passing a magic cookie that represents the work item (it could be
generated and returned to the caller when the work item is queued up).
Yes, that makes sense. Any suggestions as to what should happen to
logic exceptions? I'm inclined to let them propagate up.
This is perhaps a policy question. I think those sorts of exceptions
represents bugs in the threadpool class itself rather then a user error. In
that case I am not sure what should happen as it means that the threadpool
class has detected an internal error and it may not be able to continue to
operate.
Ah, I'm with you. That looks like it's an incomplete comment, which
suggests that I should look *very* carefully at that code. I don't like
finding bits which clearly I've left incomplete

I seem to remember
having an interesting time trying to work out all the appropriate
semantics there.
Yes, this part of the operation is very tricky. I think there is a fair bit
of work to do to ensure that it operates correctly.
I've got a freeware threadpool class that I got from a MSFT blog over a year
ago - I'll forward it to your email. It is structured very different from
yours but it may give you some ideas. To be honest I find yours much easier
to understand

.
That's actually deliberate, I *think*. (It's a while since I wrote the
code, admittedly.) I'm not convinced it was the correct decision
though. I'll think about it more with a few use cases.
Fair enough.
Personally I
hate the underscore convention - I find it makes code harder to read.
It's definitely a personal thing, but I'd need very convincing reasons
to change habits now
Very definitely personal - I love it

It makes it easy for me to tell the
difference between a field and a property.
I think it should be too, but I haven't thought about it properly. That
whole loop is far too big at the moment - it desperately needs
refactoring, but last time I tried I failed to make it any better. I'll
have another go.
This definitely non-trivial to get it right...
So each item has a priority, and the queue is sorted by priority? That
makes sense, and shouldn't be too hard to implement.
Yes, and it is very useful for a large app.
Agreed - good point and easy to implement. (In fact, I'm not sure that
actual checking is necessary - just setting the priority in each place
should do the trick, shouldn't it? I would hope that setting a thread's
priority to the one it had before isn't significantly more expensive
than checking the existing priority.)
Yes, setting it immediately before and after each worker item callback
should do the trick. I think you want to do both to ensure that the NT
thread scheduler treats each thread the same.
I don't know much about priority inversion, but this sounds pretty
tricky. Any suggestions?
I've used classes that implement this. The basic idea is that multiple
threads want to acquire a lock on a synchronization primitive, such as a
mutex. Rather then acquire/release it directly, the mutex is wrapped by a
class that initializes the mutex and which takes as an argument the base
priority that all threads attempting to acquire the lock must be at. When a
thread attempts to get the lock the mutex wrapper ensures that the thread is
executing at the correct priority level and then attempts to acquire the
lock. After releasing the lock it sets the priority back to the original
level (there's obviously more to it then that, but that's the basic idea).
This ensures that all threads that own the mutex are running at a base
priority level such that a thread that normally runs at a low priority level
will instead run at an elevated level. This prevents the system from
suspending the thread that owns the mutex in favor of a third (or more)
thread that is running at a level higher then the base level of the low
priority thread but below the priority level of the mutex.
Mmm. That sounds potentially tricky, unless I add an identification
mechanism for items. I don't want to do it by comparing parameters.
Does a separate identification mechanism (just object, which if null
means the item can't be removed, and Equals is called for each item in
the queue to check for a match) sound okay?
I think a magic cookie may be the answer here. Just return a number that is
associated with the work item and return it from the Queueing call.
Without knowing more about CER, I don't want to comment on this.
Fair enough. I've read a bit about it on Brumme's weblog but I have not yet
seen the docs on it.
Cheers
Dave