David Levine <noSpamdlevineNNTP2@wi.rr.com> wrote:[color=blue][color=green]
> > 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.[/color]
>
> Agreed. I feel this is one of those issues that does not have a single
> "correct" answer.
>[color=green]
> > 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]
>
> 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.[/color]
Unfortunately, that doesn't work due to .NET only allowing you to set
the thread name once :(
[color=blue][color=green]
> > Yikes. That involves asking the delegate for the invocation list and
> > doing each invocation myself, correct?[/color]
>
> 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);
> }
> }
> }
> }
> }
> }[/color]
Unfortunately it's not that simple, for the reasons you state later.
Given that all of this is entirely different to the way .NET treats
event handlers the rest of the time, I think I'll go for something like
this:
1) Implement the idea of cancelling further processing of the event - I
like that.
2) Use the normal .NET semantics of "whatever was subscribed when the
event is fired, use that" - don't try to react to changes during the
event.
3) Use the normal .NET semantics of an exception stopping further
processing.
4) Add some sort of exception logging for this and the other events.
<snip>
[color=blue]
> 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.[/color]
Not with the current code. I don't hold the event lock while executing
the callback.
[color=blue]
> 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.[/color]
Yup. That's where things get tricky. I think it *could* be implemented
using a very clever linked list to track the whole invocation list
(never actually combining any delegates) but I don't think it's worth
it, to be honest.
[color=blue][color=green][color=darkred]
> >> 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 ThreadController.)[/color]
>
> I haven't seen that class yet. Perhaps you could copy the documentation over
> to this class.[/color]
I'll certainly revise all the documentation when I've got the semantics
above implemented - and probably implement and document the same
semantics in ThreadController.
[color=blue][color=green]
> > 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.[/color]
>
> 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.[/color]
Yup, sounds good to me.
[color=blue][color=green]
> > 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.[/color]
>
> 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.[/color]
Yup.
[color=blue][color=green]
> > 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.[/color]
>
> 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);
> }
> }[/color]
Right. Presumably that means the code should be compiled with TRACE and
DEBUG defined, right? Otherwise those calls are removed entirely, I
believe. That's the other thing about libraries - having a debug
version and a separate release version is often a pain, IMO :(
<snip>
[Exception handler getting parameters][color=blue][color=green]
> > 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]
>
> I don't think that matters much because if an exception occurs it is already
> going down a sub-optimal code path anyway.[/color]
But the problem is, you've got to keep the parameters around *whether
or not* an exception is thrown, just in case an exception is thrown.
[color=blue][color=green][color=darkred]
> >> 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]
>
> 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.[/color]
Right, that's not a bad idea. I haven't used the event log much - I'll
investigate.
[color=blue][color=green][color=darkred]
> >> 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.[/color]
> >
> > That's a nice idea. Again, passing in the work item which is about to
> > be executed, right?[/color]
> 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).[/color]
I think I prefer the idea of the caller optionally providing it - they
can then give something useful if they want to, and just ignore it if
they're not going to use the facility anyway.
[color=blue][color=green][color=darkred]
> >> 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.[/color]
> >
> > Yes, that makes sense. Any suggestions as to what should happen to
> > logic exceptions? I'm inclined to let them propagate up.[/color]
>
> 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.[/color]
I think letting the exception propagate is probably the way forward -
it's a shame I can't detect whether or not AppDomain.UnhandledException
has a handler attached already...
[color=blue][color=green]
> > 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]
> 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 :-).[/color]
Received - I'll have a look. I think the problem is that there's a
potential race condition just about whatever you do. I think I'll try
to fix it so it creates threads in fewer cases, but when in doubt it
should create one. I suspect the logic is "when I add the item to the
queue, was it empty before? If so, start a thread if none are running.
Otherwise, start a thread if fewer than the maximum number of threads
are running."
<snip>
[color=blue][color=green]
> > 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]
>
> Very definitely personal - I love it :-) It makes it easy for me to tell the
> difference between a field and a property.[/color]
I do that with case instead - apart from constants which are in Pascal
case.
[color=blue][color=green]
> > 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]
>
> This definitely non-trivial to get it right...[/color]
Yup. I think I can probably factor out the "work out how long to wait
for" code, which would help.
<snip>
[color=blue][color=green][color=darkred]
> >> 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]
> 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.[/color]
I don't think I want to set it before the worker item callback itself -
I want to set it before the "before work item" event is raised, so that
if that decides to change the priority, it can. I'll set it to the
default again after the "after work item" event is raised. Sound
reasonable?
[color=blue][color=green][color=darkred]
> >> Related to this, priority inversion detection/prevention.[/color]
> >
> > I don't know much about priority inversion, but this sounds pretty
> > tricky. Any suggestions?[/color]
>
> 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.[/color]
Right. I can see how that works, but I'm not sure
[color=blue][color=green][color=darkred]
> >> 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]
>
> 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.[/color]
Again, I think I prefer the idea of the client providing the
identification, but after that it's fine.
[color=blue][color=green][color=darkred]
> >> 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.
> >[/color]
> Fair enough. I've read a bit about it on Brumme's weblog but I have not yet
> seen the docs on it.[/color]
Right. I'll post again when I've implemented the changes and uploaded
them.
--
Jon Skeet - <skeet@pobox.com>
http://www.pobox.com/~skeet
If replying to the group, please do not mail me too
