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