thread pooling and short lived threads

[AlexeiOst]

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

[Sriram Krishnan]

Re: thread pooling and short lived threads

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!

--
Sriram Krishnan

http://www.dotnetjunkies.com/weblog/sriram

"AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
news:79398E16-5E8C-46D0-B7EB-D3ACB7CF06C6@mi crosoft.com...[color=blue]
> 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[/color]

[AlexeiOst]

Re: thread pooling and short lived threads

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

"Sriram Krishnan" wrote:
[color=blue]
> 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!
>
> --
> Sriram Krishnan
>
> http://www.dotnetjunkies.com/weblog/sriram
>
>
> "AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
> news:79398E16-5E8C-46D0-B7EB-D3ACB7CF06C6@mi crosoft.com...[color=green]
> > 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[/color]
>
>
>[/color]

[Sriram Krishnan]

Re: thread pooling and short lived threads

Unfortunately - you consider it a bug while Microsoft says that is 'by
design'. So you're stuck I'm afraid. You might want to check out
http://www.bearcanyon.com/dotnet/#tpcontrol for an example of how to
increase the pool thread limit

--
Sriram Krishnan

http://www.dotnetjunkies.com/weblog/sriram

"AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
news:3DC0A858-AAB9-430E-9D6F-2ED153D61BFB@mi crosoft.com...[color=blue]
> 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
>
> "Sriram Krishnan" wrote:
>[color=green]
>> 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!
>>
>> --
>> Sriram Krishnan
>>
>> http://www.dotnetjunkies.com/weblog/sriram
>>
>>
>> "AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
>> news:79398E16-5E8C-46D0-B7EB-D3ACB7CF06C6@mi crosoft.com...[color=darkred]
>> > 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[/color]
>>
>>
>>[/color][/color]

[David Levine]

Re: thread pooling and short lived threads

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).


"AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
news:79398E16-5E8C-46D0-B7EB-D3ACB7CF06C6@mi crosoft.com...[color=blue]
> 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[/color]

[Jon Skeet [C# MVP]]

Re: thread pooling and short lived threads

David Levine <noSpamdlevineN NTP2@wi.rr.com> wrote:[color=blue]
> 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).[/color]

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

--
Jon Skeet - <skeet@pobox.co m>

http://www.pobox.com/~skeet

If replying to the group, please do not mail me too

[Willy Denoyette [MVP]]

Re: thread pooling and short lived threads

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.

"David Levine" <noSpamdlevineN NTP2@wi.rr.com> wrote in message
news:%23xpmG7cs EHA.2556@tk2msf tngp13.phx.gbl. ..[color=blue]
> 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).
>
>
> "AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
> news:79398E16-5E8C-46D0-B7EB-D3ACB7CF06C6@mi crosoft.com...[color=green]
>> 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[/color]
>
>[/color]

[David Levine]

Re: thread pooling and short lived threads

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?

"Willy Denoyette [MVP]" <willy.denoyett e@pandora.be> wrote in message
news:eS1HA%23js EHA.2808@TK2MSF TNGP14.phx.gbl. ..[color=blue]
> 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.
>
> "David Levine" <noSpamdlevineN NTP2@wi.rr.com> wrote in message
> news:%23xpmG7cs EHA.2556@tk2msf tngp13.phx.gbl. ..[color=green]
>> 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).
>>
>>
>> "AlexeiOst" <AlexeiOst@disc ussions.microso ft.com> wrote in message
>> news:79398E16-5E8C-46D0-B7EB-D3ACB7CF06C6@mi crosoft.com...[color=darkred]
>>> 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[/color]
>>
>>[/color]
>
>[/color]

[David Levine]

Re: thread pooling and short lived threads

>[color=blue]
> 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
>[/color]
I'll take a look at it - I'm always on the lookout for good open source
libs. Thanks.

[Jon Skeet [C# MVP]]

Re: thread pooling and short lived threads

David Levine <noSpamdlevineN NTP2@wi.rr.com> wrote:[color=blue][color=green]
> > 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[/color][/color]
[color=blue]
> I'll take a look at it - I'm always on the lookout for good open source
> libs. Thanks.[/color]

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.

--
Jon Skeet - <skeet@pobox.co m>

http://www.pobox.com/~skeet

If replying to the group, please do not mail me too

[David Levine]

Jon Skeet's thread pooling sample

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
exceptionHandle r 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 ExceptionHandle r(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 programmaticall y 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 SwallowExceptio n. 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 OnBeforeWorkIte m, 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
WorkerThreadLoo p(), 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 WorkerThreadLoo p (line 600), the LOC: Monitor.Wait(qu eueLock,idlePer iod);
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



"Jon Skeet [C# MVP]" <skeet@pobox.co m> wrote in message
news:MPG.1bd986 fc86ff39fd98b6e 6@msnews.micros oft.com...[color=blue]
> David Levine <noSpamdlevineN NTP2@wi.rr.com> wrote:[color=green][color=darkred]
>> > 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[/color][/color]
>[color=green]
>> I'll take a look at it - I'm always on the lookout for good open source
>> libs. Thanks.[/color]
>
> 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.
>
> --
> Jon Skeet - <skeet@pobox.co m>
> http://www.pobox.com/~skeet
> If replying to the group, please do not mail me too[/color]

[Jon Skeet [C# MVP]]

Re: Jon Skeet's thread pooling sample

David Levine <noSpamdlevineN NTP2@wi.rr.com> wrote:[color=blue]
> 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![/color]

Tell me about it :)

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

I think it highlights a few genuine implementation deficiencies, myself
:)
[color=blue]
> 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.[/color]

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.
[color=blue]
> 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.[/color]

Spot on - which behaviour do you think would be better?
[color=blue]
> Processing the OnException event #1: it locks around making a copy of the
> exceptionHandle r 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 ExceptionHandle r(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.[/color]

Ooh, nasty. I hadn't thought of that.
[color=blue]
> 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 programmaticall y 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.[/color]

Yikes. That involves asking the delegate for the invocation list and
doing each invocation myself, correct?
[color=blue]
> 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.[/color]

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 ThreadControlle r.)
[color=blue]
> Processing the OnException event #3: Never never never use an empty catch!
> (my own pet peeve :-)
> I usually supply a method that I call SwallowExceptio n. 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.[/color]

But then, do I definitely want the ThreadPool to stop if a non-
CLSCompliant exception is thrown? ThreadAbortExce ption 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 SwallowExceptio n, 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 "OnExceptionInE xceptionHandler "
route :)

Advice welcome here.
[color=blue]
> 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.[/color]

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?
[color=blue]
> 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.[/color]

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

Not entirely sure what you mean here - could you elaborate?
[color=blue]
> An ExecuteWorkItem Option: When firing the OnBeforeWorkIte m, use it to check
> if the subscriber wants to cancel the work item. Or add a cancel mechanism.[/color]

That's a nice idea. Again, passing in the work item which is about to
be executed, right?
[color=blue]
> ExecuteWorkItem : This is called from within a large loop in method
> WorkerThreadLoo p(), 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.[/color]

Yes, that makes sense. Any suggestions as to what should happen to
logic exceptions? I'm inclined to let them propagate up.
[color=blue]
> 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.[/color]

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.
[color=blue]
> 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.[/color]

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.
[color=blue]
> In WorkerThreadLoo p (line 600), the LOC: Monitor.Wait(qu eueLock,idlePer iod);
> 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.[/color]

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 :)
[color=blue]
> 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...[/color]

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.
[color=blue]
> 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.[/color]

So each item has a priority, and the queue is sorted by priority? That
makes sense, and shouldn't be too hard to implement.
[color=blue]
> 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.[/color]

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.)
[color=blue]
> Related to this, priority inversion detection/prevention.[/color]

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

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

Without knowing more about CER, I don't want to comment on this.

--
Jon Skeet - <skeet@pobox.co m>

http://www.pobox.com/~skeet

If replying to the group, please do not mail me too

[Jon Skeet [C# MVP]]

Re: Jon Skeet's thread pooling sample

Jon Skeet [C# MVP] <skeet@pobox.co m> wrote:[color=blue][color=green]
> > In WorkerThreadLoo p (line 600), the LOC: Monitor.Wait(qu eueLock,idlePer iod);
> > 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.[/color]
>
> 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 :)[/color]

<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...

--
Jon Skeet - <skeet@pobox.co m>

http://www.pobox.com/~skeet

If replying to the group, please do not mail me too

[Jon Skeet [C# MVP]]

Re: Jon Skeet's thread pooling sample

David Levine <noSpamdlevineN NTP2@wi.rr.com> wrote:[color=blue]
> 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.[/color]

<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?

--
Jon Skeet - <skeet@pobox.co m>

http://www.pobox.com/~skeet

If replying to the group, please do not mail me too

[David Levine]

Re: Jon Skeet's thread pooling sample

>> When setting MaxThreads, if the new MaxThreads count is less then[color=blue][color=green]
>> MinThreads, either throw an exception or set the value of MinThreads to
>> that
>> of MaxThreads.[/color]
>
> <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?
>[/color]
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.