thread pooling and short lived threads

  • Thread starter Thread starter Guest
  • Start date Start date
G

Guest

Everywhere in documentation there are recommendations to use threads from
thread pooling for relatively short tasks. As I understand, fetching a page
or multiple pages (sometimes up to 50 but not tipical) from the Internet and
doing some processing on those would be considered to be a big/long task for
a thread from a pool. In our app it is possible to break the task into some
small ones (thread per fetch and processing thereafter or event smaller), but
the more threads we create the more context switches we get. Where is the
ballance point? What penalties do we incur for big/long tasks?

One more thing. There seems to be a bug with large numbers of asynchronous
IO where threads deadlock and timeout (it is all over the developers
communities). Any info on when the fix is going to be out?

Thank you,

Alexei
 
You may want to be real careful about using the ThreadPool for downloading
stuff using WebRequest classes as they use the ThreadPool too. I had a lot
of pain figuring this out - not fun!
 
Thank you for your warning. Unfortunatelly, I bumped into this problem
already as I mentioned in the 'One more thing' part of the message. Or is
there something else that you saw?

Thank you,

Alexei
 
The threadpool is a system wide resource that by default provides 25
threads. The BCL uses this internally for many of its operations so if you
are making heavy use of it yourself in long-running tasks there is a danger
that you will exhaust all available threads.

However, it does sound like your processing requirements are good candidates
for a threadpool. There are plenty of opensource threadpool classes
available that you can use for those tasks; use google to locate one. This
will give you complete control over the number of threads in the threadpool
and how they behave, and you can customize it for your own purposes (e.g.
tasks of different priorities, etc).
 
David Levine said:
The threadpool is a system wide resource that by default provides 25
threads. The BCL uses this internally for many of its operations so if you
are making heavy use of it yourself in long-running tasks there is a danger
that you will exhaust all available threads.

However, it does sound like your processing requirements are good candidates
for a threadpool. There are plenty of opensource threadpool classes
available that you can use for those tasks; use google to locate one. This
will give you complete control over the number of threads in the threadpool
and how they behave, and you can customize it for your own purposes (e.g.
tasks of different priorities, etc).

I have one which you're welcome to use, for example. It's part of my
MiscUtil library available at
http://www.pobox.com/~skeet/csharp/miscutil
 
The threadpool is not a system wide resource, it's a CLR (aka. per process)
managed pool of:
1) max. 25 threads per CPU plus
2) a pool of max 1000 IOCP threads per process.

Willy.
 
I stated it badly...by "system" I was referring to the managed process. I
was unaware of the 1000 IOCP threads/process; is this a Windows limit or a
CLR limit?
 
I'll take a look at it - I'm always on the lookout for good open source
libs. Thanks.

Goodo - it hasn't had much use yet, so although I'm fairly sure it's
thread-safe, there may be other features which would be useful to add.
 
Here are some comments on the threadpool; please keep in mind that this is
very preliminary and does not really evaluate it fully, especially in terms
of its correctness - analyzing the correctness of multithreaded behavior is
very difficult!

Also, I suggest several options as ways that it could be extended - it in no
way suggests the current implementation has any deficiencies.

When setting the property of the Name, you might check to see if it has
already been initialized, and if it has throw an exception (similar to
current .net functionality). This is more a question of how it should behave
then - the code isn't wrong.

The value for MinThreads should not be allowed to exceed the value of
MaxThreads. Either throw an exception or increase the value of MaxThreads to
that of MinThreads.

When setting MaxThreads, if the new MaxThreads count is less then
MinThreads, either throw an exception or set the value of MinThreads to that
of MaxThreads.

Processing the OnException event #1: it locks around making a copy of the
exceptionHandler event, and outside of that lock it iterates the list and
forwards the exception to all subscribers (calls: eh(this,e)). This brings
up the following scenario:
1. OnException calls eh(this,e)
2. The invoked handler wants to remove a different handler from the list,
perhaps because the exception has invalidated some other logic.
3. It calls WorkerException -= new ExceptionHandler(yada);

This will remove the handler from the actual event but not from the event
that is being iterated. The result is that the handler will actually be
called even though the subscriber believes it is no longer subscribed.

There are a couple of ways to handle this: first, you could define this as
"bad" and a "good" programmer should not do this - e caveat programmer -
and you deserve whatever you get. You could programmatically handle this by
adding logic to detect if a remove occurs while iterating the subscriber
list, and then to skip delegates that have been removed during the
callback - there should be ways to synchronize this so that even twisted
code paths will be handled correctly. If the subscriber removes itself in
its own callback this should have no impact.

Processing the OnException event #2: The callback probably ought to be
encapsulated in a try-catch so that a handler that throws does not abort
other subscribers to the exception event. There is a try-catch around the
entire body but then one exception terminates the entire list of callbacks.

Processing the OnException event #3: Never never never use an empty catch!
(my own pet peeve :-)
I usually supply a method that I call SwallowException. This can output the
exception to the debug port, log it, whatever, but it is not the same as
publishing it. There are times where swallowing an exception is the correct
thing to do but these should be rare. Also, when you swallow using a naked
catch you are swallowing all exceptions, including non-CLSCompliant
exceptions. It is better to use ... catch(Exception) so that it does not
blindly swallow everything.

Processing the OnException event #4: An option would be to define a handler
that uses an out bool argument to indicate if further processing should be
halted. I think it might be likely that a subscriber may only care about
exceptions that occur on a particular thread or during a particular
operation, and not want other subscribers to see or handle those events.

In addition, you may want to add an argument that passes the same arguments
that were passed to the worker item queue so that the context that the
exception occurred in can be established. Without that information it will
be difficult for the exception callback handler to determine what operation
the exception occurred in.

It would also be very useful to have a log of exceptions that occurred
during a callback.
An ExecuteWorkItem Option: When firing the OnBeforeWorkItem, use it to check
if the subscriber wants to cancel the work item. Or add a cancel mechanism.

ExecuteWorkItem: This is called from within a large loop in method
WorkerThreadLoop(), and the try-catch covers everything in that loop. I
would surround the call to ExecuteWorkItem with a local try-catch in
addition to the outer try-catch. This will distinguish between exceptions
caused by other logic and exceptions in the worker callback itself.


AddWorkItem: The comment says to start the thread before adding the item,
but the actual code starts the thread after the item has been added to the
queue and the Monitor pulsed. A couple of points: the thread probably ought
to be started first, and the code should probably check if a thread is
available for use before starting a new thread. As it is I believe it will
keep creating threads until MaxThreads is reached regardless if one is
actually needed. There could be an IdleThread count that is checked before a
new thread is created. The code as is ought to work but it could perhaps be
optimized.

In WorkerThreadLoop (line 600), the LOC: Monitor.Wait(queueLock,idlePeriod);
This checks the actual variable idlePeriod, not the property. This means
that this check occurs outside the protection of the stateLock monitor. Is
this supposed to be like this? If not, I would suggest renaming all private
fields to use a leading underscore - this makes it easier to distinguish
between internal fields and properties.

I did not evaluate the correctness of the thread control/worker dequeue
logic - a quick glance did not reveal an obvious problems - I expect it
works correctly, but this kind of code is extremely hard to analyze for
correctness. Did you analyze what happens if a worker callback throws an
abort exception? This will kill the thread because there is nothing in the
logic to reset the abort. I think it ought to be ok...

To sum it up, it looks like it ought to work; without actually testing it I
can't say for sure. Most of my comments are concerned with features that I
would like to see added, not with any actual problems in the code itself.
Other features that could be added are...

Priority queues.
Set/get the priority of the worker threads. This ought to be checked before
and after the callback so that if a callback changes the priority the
threadpool resets it back to the correct setting.
Related to this, priority inversion detection/prevention.
Cancellation logic (remove an item from the queue).


Future considerations - CER mechanism may be useful for logic in finally
blocks. I need to see documenation on this...

Cheers,
Dave
 
David Levine said:
Here are some comments on the threadpool; please keep in mind that this is
very preliminary and does not really evaluate it fully, especially in terms
of its correctness - analyzing the correctness of multithreaded behavior is
very difficult!

Tell me about it :)

Thanks ever so much for such detailed comments - it's much appreciated.
Also, I suggest several options as ways that it could be extended - it in no
way suggests the current implementation has any deficiencies.

I think it highlights a few genuine implementation deficiencies, myself
:)
When setting the property of the Name, you might check to see if it has
already been initialized, and if it has throw an exception (similar to
current .net functionality). This is more a question of how it should behave
then - the code isn't wrong.

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.

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.
The value for MinThreads should not be allowed to exceed the value of
MaxThreads. Either throw an exception or increase the value of MaxThreads to
that of MinThreads.

When setting MaxThreads, if the new MaxThreads count is less then
MinThreads, either throw an exception or set the value of MinThreads to that
of MaxThreads.

Spot on - which behaviour do you think would be better?
Processing the OnException event #1: it locks around making a copy of the
exceptionHandler event, and outside of that lock it iterates the list and
forwards the exception to all subscribers (calls: eh(this,e)). This brings
up the following scenario:
1. OnException calls eh(this,e)
2. The invoked handler wants to remove a different handler from the list,
perhaps because the exception has invalidated some other logic.
3. It calls WorkerException -= new ExceptionHandler(yada);

This will remove the handler from the actual event but not from the event
that is being iterated. The result is that the handler will actually be
called even though the subscriber believes it is no longer subscribed.

Ooh, nasty. I hadn't thought of that.
There are a couple of ways to handle this: first, you could define this as
"bad" and a "good" programmer should not do this - e caveat programmer -
and you deserve whatever you get. You could programmatically handle this by
adding logic to detect if a remove occurs while iterating the subscriber
list, and then to skip delegates that have been removed during the
callback - there should be ways to synchronize this so that even twisted
code paths will be handled correctly. If the subscriber removes itself in
its own callback this should have no impact.

Yikes. That involves asking the delegate for the invocation list and
doing each invocation myself, correct?
Processing the OnException event #2: The callback probably ought to be
encapsulated in a try-catch so that a handler that throws does not abort
other subscribers to the exception event. There is a try-catch around the
entire body but then one exception terminates the entire list of callbacks.

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.)
Processing the OnException event #3: Never never never use an empty catch!
(my own pet peeve :-)
I usually supply a method that I call SwallowException. This can output the
exception to the debug port, log it, whatever, but it is not the same as
publishing it. There are times where swallowing an exception is the correct
thing to do but these should be rare. Also, when you swallow using a naked
catch you are swallowing all exceptions, including non-CLSCompliant
exceptions. It is better to use ... catch(Exception) so that it does not
blindly swallow everything.

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.

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.

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.
Processing the OnException event #4: An option would be to define a handler
that uses an out bool argument to indicate if further processing should be
halted. I think it might be likely that a subscriber may only care about
exceptions that occur on a particular thread or during a particular
operation, and not want other subscribers to see or handle those events.

Right. That's easy enough to do if I go down the manual invocation
list, but not easy otherwise. I'll think about it.

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?
In addition, you may want to add an argument that passes the same arguments
that were passed to the worker item queue so that the context that the
exception occurred in can be established. Without that information it will
be difficult for the exception callback handler to determine what operation
the exception occurred in.

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.
It would also be very useful to have a log of exceptions that occurred
during a callback.

Not entirely sure what you mean here - could you elaborate?
An ExecuteWorkItem Option: When firing the OnBeforeWorkItem, use it to check
if the subscriber wants to cancel the work item. Or add a cancel mechanism.

That's a nice idea. Again, passing in the work item which is about to
be executed, right?
ExecuteWorkItem: This is called from within a large loop in method
WorkerThreadLoop(), and the try-catch covers everything in that loop. I
would surround the call to ExecuteWorkItem with a local try-catch in
addition to the outer try-catch. This will distinguish between exceptions
caused by other logic and exceptions in the worker callback itself.

Yes, that makes sense. Any suggestions as to what should happen to
logic exceptions? I'm inclined to let them propagate up.
AddWorkItem: The comment says to start the thread before adding the item,
but the actual code starts the thread after the item has been added to the
queue and the Monitor pulsed.

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.
A couple of points: the thread probably ought
to be started first, and the code should probably check if a thread is
available for use before starting a new thread. As it is I believe it will
keep creating threads until MaxThreads is reached regardless if one is
actually needed. There could be an IdleThread count that is checked before a
new thread is created. The code as is ought to work but it could perhaps be
optimized.

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.
In WorkerThreadLoop (line 600), the LOC: Monitor.Wait(queueLock,idlePeriod);
This checks the actual variable idlePeriod, not the property. This means
that this check occurs outside the protection of the stateLock monitor. Is
this supposed to be like this? If not, I would suggest renaming all private
fields to use a leading underscore - this makes it easier to distinguish
between internal fields and properties.

I suspect it's meant to be IdlePeriod, yes. I also suspect I'd have
made the same mistake even if I'd changed the convention. 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 :)
I did not evaluate the correctness of the thread control/worker dequeue
logic - a quick glance did not reveal an obvious problems - I expect it
works correctly, but this kind of code is extremely hard to analyze for
correctness. Did you analyze what happens if a worker callback throws an
abort exception? This will kill the thread because there is nothing in the
logic to reset the abort. I think it ought to be ok...

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.
To sum it up, it looks like it ought to work; without actually testing it I
can't say for sure. Most of my comments are concerned with features that I
would like to see added, not with any actual problems in the code itself.
Other features that could be added are...

Priority queues.

So each item has a priority, and the queue is sorted by priority? That
makes sense, and shouldn't be too hard to implement.
Set/get the priority of the worker threads. This ought to be checked before
and after the callback so that if a callback changes the priority the
threadpool resets it back to the correct setting.

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.)
Related to this, priority inversion detection/prevention.

I don't know much about priority inversion, but this sounds pretty
tricky. Any suggestions?
Cancellation logic (remove an item from the queue).

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?
Future considerations - CER mechanism may be useful for logic in finally
blocks. I need to see documenation on this...

Without knowing more about CER, I don't want to comment on this.
 
Jon Skeet said:
I suspect it's meant to be IdlePeriod, yes. I also suspect I'd have
made the same mistake even if I'd changed the convention. 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 :)

<snip>

While it's a bug, the fix *isn't* to make it use the IdlePeriod
property. I believe it's actually meant to be waitPeriod instead of
idlePeriod. Put it this way - if it's not meant to be waitPeriod, I
don't know why I've bothered calculating waitPeriod to start with.

The reason I *can't* use IdlePeriod in there is that it could cause a
deadlock - that would acquire stateLock when the thread already owns
queueLock, and the reverse is true elsewhere. I've carefully documented
the order in which locks are allowed to be acquired :) Just another of
those instances where it's scarily easy to get things wrong...
 
David Levine said:
The value for MinThreads should not be allowed to exceed the value of
MaxThreads. Either throw an exception or increase the value of MaxThreads to
that of MinThreads.

When setting MaxThreads, if the new MaxThreads count is less then
MinThreads, either throw an exception or set the value of MinThreads to that
of MaxThreads.

<snip>

I think I've remembered the reason for these being independent, odd as
it may seem. If you want to set both of them, you would have to either
set one of them twice (if they were self-regulating) catch a potential
exception (if they threw exceptions) or check the current values first.
For instance, say you want to get them to be (5, 10). If they're
currently (2, 3) then you couldn't set MinThreads first. If they're
currently (12, 15) then you couldn't set MaxThreads first.

Possibly the solution to this is to have a separate method to set them
both atomically - what do you reckon?
 
When setting MaxThreads, if the new MaxThreads count is less then
<snip>

I think I've remembered the reason for these being independent, odd as
it may seem. If you want to set both of them, you would have to either
set one of them twice (if they were self-regulating) catch a potential
exception (if they threw exceptions) or check the current values first.
For instance, say you want to get them to be (5, 10). If they're
currently (2, 3) then you couldn't set MinThreads first. If they're
currently (12, 15) then you couldn't set MaxThreads first.

Possibly the solution to this is to have a separate method to set them
both atomically - what do you reckon?
I thought that might have been the reason, in which case an atomic method
that set both would do the trick, and then you could leave the other checks
in the property setter.
 
Jon Skeet said:
<snip>

While it's a bug, the fix *isn't* to make it use the IdlePeriod
property. I believe it's actually meant to be waitPeriod instead of
idlePeriod. Put it this way - if it's not meant to be waitPeriod, I
don't know why I've bothered calculating waitPeriod to start with.

The reason I *can't* use IdlePeriod in there is that it could cause a
deadlock - that would acquire stateLock when the thread already owns
queueLock, and the reverse is true elsewhere. I've carefully documented
the order in which locks are allowed to be acquired :) Just another of
those instances where it's scarily easy to get things wrong...
Yup, which is why I wasn't sure if it was right or wrong :-). Checking lock
orders is very tricky, time-consuming, and error prone. Do you recall what
the purpose of the check was?
 
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
 
David Levine said:
I thought that might have been the reason, in which case an atomic method
that set both would do the trick, and then you could leave the other checks
in the property setter.

Yup. I'll do it that way.
 
David Levine said:
Yup, which is why I wasn't sure if it was right or wrong :-). Checking lock
orders is very tricky, time-consuming, and error prone. Do you recall what
the purpose of the check was?

Not sure what you mean. I'm not sure I can get away with *not*
requiring a certain lock order, unless I have just a single lock (which
is a possibility, I suspect - I'd have to check).
 
Jon Skeet said:
Not sure what you mean. I'm not sure I can get away with *not*
requiring a certain lock order, unless I have just a single lock (which
is a possibility, I suspect - I'd have to check).

Sorry if I wasn't clear. I meant that I had not analyzed the relationship
between the stateLock and the queueLock to verify that the lock order was
guaranteed if you used the property instead of the field. I agree that you
need a certain lock order, or a single lock.
 
Back
Top