Your advice is bad. Sometimes this may be the right way,
sometimes it may not. There are many instances when
continuing to propogate the event to other listeners
when one fails is in fact NOT what you want. As always,
this is just another techinque and it should be used
where appropriate, not applied blindly across the board.
Ian:
- "But the downside is that if one of my
handlers throws an exception, I never get to find out!
"
We're talking about a publish-subscribe scenario. When
your componenet fires an event it should *never in a
million years* care what any of the subscribers do with
the event. It should't know who they are, how many are
there, and what they should and should not be doing,
whether they throw the exception, handle it, or don't do
anything. Your claim would be correct if we were talking
about a standard callback mechanism, which is a
different logical model, where you probably want to know
that your message got to all the subscribers.
- "All that saved stuff will get picked up by
the GC. And the interesting thing is that it gets picked
up *exactly* as quickly as it would do if you called
EndInvoke like you're meant to"
The GC will never clean up the the resources that are
used by the delegate inside out EventsHelper class (the
one we use to execute the
"InvokeDelegate" method with). That's
because it is static and will staty in memory for as
long as the AppDomain that holds the static class will.
And you should call EndInvoke when you *care* about the
outcome ofr your callback, which is not the case in the
event subscriber model. If you're talking about a
requirement that says "reliable messaging
needed", that's something else. But with
events, you should really jsut notify the subscribers
and move on as quickly as you can.
- "If I need to be able to notify multiple
subscribers where those subscribers are flakey, then
maybe events simply aren't the right mechanism?
"
If you need reliable messaging, and your component
should be coupled with the subscribers, yes, that's not
the model for you. This model is right for when you
can't know in advance who your subscribers will be, and
have no control over them (that is, assume they all
throw exceptions or take a long time to process, because
you can't prove they won't).
if you write a component that runs inside someone else's
application and the application does something bad (like
throwing an exception or taking too long to respond) you
should just let the application crush or stop
responding, your user should then debug the application
and fix it
if you are the application calling a plugin then it's a
totally different story and you should program
defensively, but this is not a common use of events
but the really evil thing you have done here is calling
events on another thread, the reasons are:
1. writing good multi threading safe code is really hard
2. nobody expects an event to fire on a different thread
this is a trap for programmers that are not familiar
with your style, and a really big one
what you just did is force a lot of extra complexity on
your user in the name of some in
"publish-subscribe
scenario subscribers don't care" ideal
you just made your user's work much more difficult
because you wanted a pure textbook publish-subscribe
implementation
I work with a component that eat away exceptions and
calls events in random threads in my current project,
it's the hardest component to work with I have ever
used, it forces me to write complex thread
synchronization code for what should have been a simple
single threaded application (and when that code fails it
gives no indication and just continues like nothing
happened)
this is not a good programming practice and I hope
people don't start writing code like that without a very
good reason
if you want to see how bad this is as a general practice
just think of a button class that fires it's events like
you wrote, now think about a program that changers a
label when a button is pressed (a WinForms object can
only be accessed from the thread that created it) - just
look at all the added code and complexity (and bugs)
Nir:
- "nobody expects an event to fire on a
different thread "
Unless they want any kind of asynchronous invocation,
you mean. In Winforms applications this happens all the
time. You want to make you application responsive while
performing lengthy operations so you do the operations
using the thread pool or other threading technique. What
happens when you call a web service from your winform?
Should your user just wait against an application that
is stuck for 3 seconds?
In fact, that's the main reason that in .Net 2.0 you
have the Background Worker component implementation - to
let you implement asynchronous behavior in a GUI thread
safe manner.
One of the things that will be shown in the book is how
to solve the threading questions. Unfortunately, I can't
write about it without overstepping some legalities, so
we'll leave it at that for now.
- "this is not a good programming practice and
I hope people don't start writing code
like that without a very good reason "
Multi threaded winform apps (and win32 apps) have
existed since the beginning of time, basically. Are you
saying everyone should just not use asynchronous
invocation, ever? Are you saying that when an event
fires, it should *always* be susceptible to exceptions,
lengthy operations and such from its subscribers?
Always?
What about the thousands of applications to which this
scenario is perfect?
What you're talking about is a callback and
communication mechanism (for plugins and such) where
this does not fit.
Sure, threads lead to bugs, but there's a very easy
usage scenario here for GUI thread on the form getting
the event: "If(InvokeRequired) {Invoke(MyEvent,
args);}"
BTW, there's nothing preventing you from firing the
event in a sychrounous manner if that's what your
component needs. Put a "Fire" and
another "AsyncFire" method on the
EventsHelper. One calls InvokeDelegate using
BeginInvoke, and one calls directly.
A couple of notes, based on my own recent experience:
1. BeginInvoke uses the threadpool. What happens if one
of your subscribers never returns (intentionally or not;
maybe they deadlocked)? After 25 (per processor)
BeginInvokes, all of your events -- even to other
subscribers have effectively been DOSed.
One way around this would be to spin off a separate
Thread (not a threadpool thread). Obviously it's still
possible to DOS that situation, but it will take longer,
and be easier to debug (since you'll see the absurd
number of threads in Task Manager etc, rather than just
having the events mysteriously stop). Alternatively you
could implement a timeout and Abort any thread that took
an absurdly long time to finish.
2. I can think of one situation where you might care
about detecting "dead" clients. If
you're a remoting server and you're raising remoted
events back to the client, it might be useful to detect
that a method call threw a remoting-related exception
(such as a SocketException) and remove that subscriber,
log the error, etc. Of course, removing the subscriber
can be somewhat tricky given that only the class
declaring the event can modify the subscriber list, at
least without a little additional legwork. I may write
up the solution we're using and post it on my site, as
it seems to be working out rather well.
Thanx, this was just where i was looking for. I have a
windows service which is capable of sending messages
with remoting to listeners with events. If one of these
messages fails it has to be logged and the service is
continuing delivering its messages to the other
listeners. After study of the logged error i make sure
these exceptions will not happen again. But thats why
defensive events are a very good way of programming. In
the ideal world everything is always running fine, but
we all know this is not the case. So i want to make sure
i catch all glitches the right way, and i think this is
a very good way.
Im an advocate of the fail-fast philosphy on exceptions.
I absolutely never want to gooble up exceptions the way
you proposed in your delegate invoke with try/catch.
Unless your program recognises exactly what the
exception is and can take steps to rectify the
situation, it should fall over in a heap.
If you call every delegate asynchronously, you hit the
thread pool pretty hard if you have a lot of subscribers
(and thread pool resources are precious ;-).
As an alternative you could consider calling the
FireClickedEvent asynchronously. That way you only eat
up 1 thread of the thread pool. This is of course only
an option if the event is not too time-sensitive and
subscriber would still get predictable/expected behavior
when their handler is called with a certain delay...