|
Thank you for explaining the "Do Not Catch Exceptions That You Cannot Handle" rule - perhaps MSDN or www.asp.net should put your code up as "article of the day"! It is however, not just a C# problem, but affects VB.NET (and proabably every dot net language.)
In many situations (such as a web application), there come a point going up the call stack, that the error detail must be replaced by a bland "there has been an error - please contact support.".
Manifestly, the routine that caught the error must log it in as much detail as neccessary (preferably to a custom event log) and then set an error return code. As this error return, ascends the call stack, various other pieces of information may need to be logged as warning messages to the event log.
|
|
|
|
|
After reading through the article and the various posts replying to it, I can see the usefulness of the IsCritical approach within any given class library, however, the comments on hiding the exceptions are quite valid. I ended up inadvertently hiding an exception within a library in a recent project and it took ages to track down the problem.
An alternative approach here would be to do something like this...
class MyLibraryException : System.Exception<br />
{<br />
public MyLibraryException(Exception innerException)<br />
{ <br />
if (IsCritical(innerException))<br />
{<br />
isCritical = true;<br />
}<br />
else<br />
{<br />
isCritical = false;<br />
}<br />
}<br />
<br />
private bool isCritical;<br />
<br />
public bool IsCritical<br />
{<br />
get { return isCritical; }<br />
set { isCritical = value; }<br />
}<br />
}
This kind of root exception then provides the recommendation of criticality, according to the library, to the application developer but it doesn't constrain the developer or hide any information from them.
This solution relies on effectively catching everything and rethrowing at least a MyLibraryException. This can then be interrogated for criticality and the nature of the innerException. The app developer can then do whatever they want with the caught exception, including throwing the innerException.
Keith Jackson
Developer
S-Cool Ltd.
|
|
|
|
|
I agree with your point of view. There are situations when it is not doable to catch all exceptions explicitly, and where the "any unknow exception is fatal" approach is not feasible, so it should be possible to deviate from the rule.
However, I do not quite agree with the list of exceptions that you check for criticalness.
BadImageFormatException: This exception is thrown when unmanaged code is passed to Load for loading. It can be used to check if a file is an assembly, there is a Microsoft How to example for that somewhere.
AppDomainUnloadedException: This one happens when you create a new application domain, unload it again and try to access it afterwards. This shows a problem with the application logic.
CannotUnloadAppDomainException: It just says that you cannot unload the application domain for various reasons.
These three are not critical in the sense that the rest of the application could not proceed. Of course, they can show problems with your 'unattended job loading and execution code', but still we can try to proceed with the next job, especially if we think of them as being caused by such a job.
InvalidProgramException: Even this one is not critical in all situations. It depends: If it is happening in your code, it is very critical. If it is happening in the job to be carried out, possibly after loading a plug-in that was built with an unknown tool, just state failure of this job and continue.
ThreadAbortException: The documentation says "ThreadAbortException is a special exception that can be caught, but it will automatically be raised again at the end of the catch block". But it certainly does not hurt to have it here.
StackOverflowException: This one is not in your list, and for 2.0 it is not necessary: "Starting with the .NET Framework version 2.0, a StackOverflowException object cannot be caught by a try-catch block and the corresponding process is terminated by default. " You might want to add it for version 1.1 code, though.
So the only two really critical ones are ExecutionEngineException and OutOfMemoryException.
The latter one can be avoided with the new MemoryFailPoint class.
|
|
|
|
|
I'm suprised that you didn't list ThreadAbortException as critical. If you catch that exception in a backgrond thread, then your application will not close. It will look closed, because the main form will disappear, but the app will still exist in the task list.
|
|
|
|
|
You're right. I should have added ThreadAbortException. I will update this article with this exception as soon as possible. Thanks for your feedback.
|
|
|
|
|
When writing unattended apps, I have had a problem with exception dialogs appearing on unhandled or unanticipated exceptions. When there is no user present, this may halt a system and prevent a possible recovery. I have tried many different APIS to catch all exceptions and simply log them or e-mail them and then terminate and restart the system. Somehow, there is always an exception that manages to "squeak by" and display a dialog box. I finally had to install my own CLR exception handler at the OS level because I could not always stop exception dialogs from coming up.
Granted, this does not solve extremely unrecoverable errors like disk full, but it at least allows an unattended system to have the change of staying up and recovering vs. sitting there waiting for someone to press OK to continue.
Although most people would say don't write code that causes exceptions, the fact is that you cannot account for all exceptions, especially if you are using 3rd party assemblies.
|
|
|
|
|
Hi,
Can you elaborate or post some code if possible on "Installing own CLR exception handler at the OS level". Seems it would be very helpful in one of our applications.
Thanks
Sujith Jagini
|
|
|
|
|
well i'm not sure what varnk meant, but here are some suggestions.
-- There are some events you could add a handler to, such as System.Windows.Forms.Application.ThreadException and System.AppDomain.CurrentDomain.UnhandledException .
-- In your main loop:
<br />
static void Main() {<br />
try {<br />
Application.Run(new MainForm());<br />
} catch ...<br />
}<br />
|
|
|
|
|
First, I've noticed that the MFC designers don't follow the "don't eat exceptions you don't understand" rule. Their windows message handler catches CException, calls ReportError on it, and then deletes it. It seems that even the MFC team feels that the rule is not doable.
I've also found several situations in which displaying a message box is not an acceptable solution. If the code for drawing on a window throws an exception, for instance, a message box would allow the window to be drawn again before the user can press OK. However, one can still draw the text of the exception, and in C#, one could presumeably draw the stack trace as well. Another place is in ON_UPDATE_COMMAND_UI handlers. I haven't found much solution for that except to eat the exceptions.
The simple solution of loging info about exceptions that occur in a batch process seems at least as reasonable as MFC aproach of displaying a message box for anything that goes wrong. In fact, I think it's more so, since loging info about an exception won't cause the same exception to occur again.
Nathan Holt
|
|
|
|
|
It's nice to know that Microsoft doesn't follow their own rules. I've seen that before. Try running Microsoft FxCop against Application Blocks written by Microsoft and see if they pass.
I don't really know MFC. Does it have any code that does show the 'good'-way of doing things? Perhaps we can convert that to C#...
|
|
|
|
|
Michael Vanhoutte wrote:
I don't really know MFC. Does it have any code that does show the 'good'-way of doing things? Perhaps we can convert that to C#...
I don't know myself. While I've used MFC for years, I haven't studied the examples much. I usually look up classes and methods in the MSDN library.
Nathan Holt
|
|
|
|
|
My understanding is that they are trying to clean up the old C++ MFC behavior as they continue to replace it with Windows.Forms. They recognize that the specific problem you mention in the message handler is considered a bug and they're planning to fix it. From the June 2004 MSDN article "Unexpected Errors in Managed Applications":
http://msdn.microsoft.com/msdnmag/issues/04/06/NET/default.aspx
In certain cases, exceptions
that are unhandled are swallowed whole by the CLR
(although this behavior is changing in the .NET
Framework 2.0). Here is a summary of the unhandled
exception handler default behaviors that are
executing in this application.
- Unhandled exceptions that occur on the
application's main thread cause the application
to terminate. - Unhandled exceptions that occur in threads
other than the application's main thread are
swallowed by the CLR. This includes manual
threads, thread pool threads, and the CLR's
finalizer thread. If your application is a
console application, the CLR outputs exception
text to the console (though your application
keeps running). If your application is not a
console application, there is no outward
indication when an exception occurs and your
application keeps running. - Unhandled exceptions that occur on a thread
that is pumping window messages via the Windows
Forms classes are subject to the Windows Forms
unhandled exception handler. A debug dialog box
is produced by default, but this behavior can be
overridden (more on this in a moment).
Byron Jenings
|
|
|
|
|
It seems to me that a new system could create a different kind of critical error, which your function wouldn't have caught.
Perhaps a part of the problem is that the exception hierachy doesn't provide a way to distinguish minor exceptions from critical ones. I seem to recall that the standard C++ library exception system does have a badexception class or something like it that could be checked for. On the other hand, what a library designer thinks is a bad exception may not be what you think is a bad exception. I've had to deal with annoying exceptions thrown if I move to the beginning of an empty recordset.
Nathan Holt
|
|
|
|
|
A badexception-class would solve some of my problems but I agree that there could be some discussion between developers as to what a critical exception is. To me a critical exception is one that I can't possibly recover from gracefully. For example if my dll is corrupt. I personally would think that 'out of memory' is also a critical exception, but some application might decide to clear their cache when this exception occurs. That is an 'advantage' of the IsCritical-method: you can decide for yourself...
Still I agree, it's far from ideal.
|
|
|
|
|
We have the same problem with VB6. And it's worst because it throws error numbers rather than exception classes. I came with the solution to build an error handler library. When an error occurs, I call a static method named "TrapErr". Once an error is trapped, I can do some operations then decide to rethrow the error to the calling procedure or to add an entry in the log file or to display a message box with the possibility to see the details of the error. I also came with the conclusion that each errors that occurs in a component should always be treated internally and that component should always throw its own errors to the clients, with desired detailed informations on the error passed back. This worked very well for me.
|
|
|
|
|
I'm assuming that this also means that you might swallow an 'out of memory'-exception in your code?
|
|
|
|
|
Yes and no. If the form fails to display, it displays a message box instead. In the case of a stack overflow, the library could never be called, naturally. If there is no enough resources, the library could fail to do its job. I could prevent this by allocating the resources at the beginning of the program, but it never happens to be a common error. In either case the program will fail though.
|
|
|
|
|
Personally, I disagree with your statement that it is not doable to catch all of the exceptions explicitly. But, for the moment let’s say you are correct.
The one problem that I see with your way of catching and checking errors could still mask a problem. If there is a problem in the code that you are calling, whether it is a Microsoft method, a third party method, or one of your own methods and if the called method does not throw an expected exception then the code checking for the exceptions will still mask the error.
If you must generically catch exceptions, then you should look for the exceptions that you expect and wish to ignore (the non-critical ones). Any other exception that is given to you then must be a critical one regardless if it was in the documentation or not.
Just my opinion.
Jim
|
|
|
|
|
Jim, thank you very much for your feedback. It's always interesting to hear somebody else's opinion about this matter.
I agree that you should always attempt to catch specific exceptions. Needless to say that I've given this matter a great deal of thought before I came up with the 'solution' that I described above. Let me give you a specific scenario of where I would use it and I would really appreciate any insights on how you might handle this situation.
Recently, I've started converting an API that I've written in VB6 to C#. The VB6-API has about 150.000 lines of code and about 2000 properties, methods, events... You can summarize all this as: "a lot of different things can go wrong" . Another thing that this application has is a Maintenance-module. The purpose of this module is to execute long-running tasks during the night. It has two important advantages:
1. Everything that you can do using our API can also be done using this maintenance module.
2. A customer can also extend this maintenance module using his own custom code.
So, in the maintenance module you have the following code (simplified naturally):
foreach (MaintenanceTask objTask in AllMaintenanceTasks) {
try {
objTask.Execute();
} catch (Exception ex) {
Log(objTask.Name + " failed to execute.");
}
}
Needless to say that objTask.Execute could theoretically throw hundreds of different exceptions since every exception that could possibly be thrown in our API could be thrown by objTask.Execute(). And because customers extend objTask.Execute() with their own code, I don't even know at design-time all the exceptions that objTask.Execute() could throw. Naturally, I want my maintenance-module to continue running regardless of what went wrong in objTask.Execute(), unless that it is a really, really, really important problem.
How would you handle this scenario?
Or another more simple example: One of the methods of my API is called Application.Login; it can be used by a user to log into a database. In this method, the following problems cause exceptions to be thrown: invalid license key, invalid username, can't connect to database, invalid database, wrong password, filesystem io-error, .NET remoting errors...
Somebody using our Login-method just wants to know if his logged in or not. Should I expect all my customers to catch all these exceptions individually? That first of all complicates development for these customers a lot. And, they might have problems if I need to throw yet another exception in a newer version because they wouldn't catch it.
As a workaround, I could put all the code of my Login-method in a try-catch-block and throw a more generic UnableToLogin-exception for everything that can go wrong. However, I don't really want to do that because I loose a lot of detail then.
Again, I don't know of a good way to handle this situation. How would you?
|
|
|
|
|
Michael,
For your first scenario the follow may address that.
I am OK with throwing out the argument of needing to explicitly catch every different kind of exception.
That leaves how to generically handle exceptions and more importantly what to do with unexpected exceptions. My personal opinion would still be to test for the exceptions that I don't want to "error out" on and treat everything else as fatal. Your philosophy is to only look for the errors you consider fatal and treat everything else as a non-fatal, documented or not. That's OK as long as you understand you may be masking problems. If that is not an issue or is needed with a given application, then your method works just fine.
If your code is a library that I may use in my own code, personally, I would not appreciate the errors being masked out and I would ask for an option for it not to do that. If on the other hand your code is run by your company and it is used to run other peoples' code in this MaintenanceTask module, then what you are doing may be exactly appropriate. My only comment on that is you should at least print out the stack trace somewhere so it can be reviewed by someone so those other customers can be told that their code failed and where it failed. I understand that the routine you presented above has been simplified and you may already be doing that.
Your design philosophy is just different than mine. I would rather have the job error out on something unexpected and have me evaluate if it should of errored out at that point and make a coding change at that time if it is needed. Call me a "control freak"
Since my design philosophy has a different focus than yours, what I do in practice when I write a "batch" job in c# is a little different. I will write my code with very few error handlers, usually just one. In my main I will add one generic try-catch statement that makes no distinction as to what the exception is (OK, I have one exception to that, but I will discuss that later). All this generic handler does is trap the exception and print out the error message, the stack trace, and exits with a non-zero return code for our job scheduler to pick up and tell the operators that the job failed. We always deploy a .pdb file with our apps so that the stack traces will tell us which line of code failed. You just need to modify the project properties to generate one for "Release" code, it's done by default for "Debug" code. Now, if my code cares about trapping for a particular exception, then I will code for that particular exception and handle the condition or re-throw the exception.
OK, now for those who have not yet fallen asleep reading this, my one exception to my generic exception handler is for SQL errors. We use MS SQL Server extensively and I will catch those errors in my main also. I do this as a separate handler because in addition to being able to print the error message and stack trace the SQL exception object also will tell me what line of code in the Stored Proc failed, which is also very helpful.
As far as your second scenario, that one is a little trickier. I will assume the focus of this is from a library where you are throwing the errors and someone else has to catch them. I have to deal with this issue myself. We purchased a third party VB6 COM library a while back. In that code the routines just set a return code and I need to check the code in context of what is happening. Now since we have "upgraded" to their .NET version of the same library they are throwing about ten different kinds of exception objects. Personally, I would have preferred a little more of a "happy medium" on this one and to have fewer objects to catch and have a richer object to deal with that would tell me what went wrong.
More specifically in your case, unless you have a good reason to catch specific errors, like ones that are truly expected and non-fatal, I think you should just let errors percolate through. Leave it up to the developer using your library to handle the errors. If you are distributing code to the outside world and you are deliberately "hiding" errors by not re-throwing them just because you don't know what they are, then my guess is when things don't work as expected it is going to be much harder for them and you to track down the real problem during the support call. As far as the error objects you create and throw, yes, don't have too many different types. Make fewer (or one) error object and back it up with enough attributes to fully describe the problem to the caller.
Again, this is just my opinion and I don't know all of the details of your programs.
Good Luck,
Jim
|
|
|
|
|
very interesting articles which makes me think a lot.
yep, if you have a batch processor, you want it to run, no matter what, but if you catch (almost) all exception you could have a big problem masked, mhh. how to handle that ?
but there is a solution exactly for that in the API I just realized!
in the old days you will have a batch processor as a process which would spawn child process for each maintenance task.
but here comes .NET and you have application domain !
your main loop should be in one application domain, and each maintenance task in its own.
what about that, hey ?
Moreover ApplicationDomain have been created exactly to solve such issues !
|
|
|
|
|
I don't think that I fully understand your application-domain-solution. I understand that having your main loop in one application domain and having your maintenance tasks in separate application domains solves some problems: If you maintenance task crashes, than it will only be that application domain that crashes and not the main loop. But how does that solve the exception-dilema? Not all 'critical' exceptions are so bad that they cause your application domain to crash. Some (like 'out of memory') are also very important but will not (to my knowledge, I might be wrong) stop your application domain from running. So how you deal with that?
But, let's assume for now that this application domain-solution solves my maintenance-task problem. I doesn't solve the Application.Login-problem that I described earlier.
I personally don't think that the solution Jim (jfos) suggested using pdb-files is feasible for us because we're going to obfuscate our library. I don't think that the pdb-files will still work.
Jim also stated in his latest response that I should let exceptions percolate through so that they developers using my library can decide what to do with it. I see three ways of doing this:
Solution 1
----------
Either my code does this:
public class Application {
void Login(string username, string password) {
ConnectToDatabase();
VerifyLicenseKey();
ConnectToDotNetRemotingServer();
...
}
}
in which case a user might receive ApplicationExceptions that I trew because the license key was invalid for example. But he might also receive ADO.NET exceptions because I had problems with the database or he might get .NET Remoting Exceptions... The disadvantages of this according to me are:
1. Documenting all the exceptions that this method could throw is A LOT of work for me to do. I don't only have to document all exceptions thrown by Application.Login but also every exception that can possibly be thrown by any of the methods that Application.Login calls!
2. Trapping all these exceptions is also a lot of work for a user of my library. Most of the time, as a developer, they don't even want to know why it failed because they can't do much about it. They just want to know if the login was successful or not. I personally feel that I can't expect from my customers to catch 5 or even 10 different exceptions simply to check if a login was successful.
3. The advantage of using a business-layer (like my library is) is that you hide most of the complexity from users using your business-layer. I've decided to use .NET Remoting in this version, but if I run into problems with it later on, I might decide to use Memory Mapped Files in the next version of my library. Technically, this shouldn't have any impact on my users because they only call Application.Login; they don't even know that I use .NET Remoting. But the problem is that I used to let .NET
Remoting exceptions bubble all the way up so they had to catch these exceptions! If they would start using my newest version, their exception handler suddenly becomes useless.
Solution 2
----------
Another thing that I could do is this:
public class Application {
void Login(string username, string password) {
try {
ConnectToDatabase();
VerifyLicenseKey();
ConnectToDotNetRemotingServer();
...
} catch (Exception ex) {
throw new MyApplicationException("Unable to login", ex);
}
}
}
This would be the easiest solution for users of my library. They just have to catch MyApplicationException to know if they've logged in or not. But this also has two disadvantages:
1. I hide some of the details of the exception. Using my previous code, a user might receive a 'can't connect to database'-exception which is cool because they immediately know what's wrong. They can also write your code to handle exceptions in a specific way.
2. The biggest danger with this way of working is that it hides important exceptions again. So, I can't do that.
Solution 3
----------
Another thing I could do is:
public class Application {
void Login(string username, string password) {
try {
ConnectToDatabase();
VerifyLicenseKey();
ConnectToDotNetRemotingServer();
...
} catch (Exception ex) {
if IsCritical(ex) throw;
throw new MyApplicationException("Unable to login", ex);
}
}
}
This has the advantage that any critical exception simply walks up the stack, but any other exception is wrapped in a MyApplicationException. Users of my library can now safely catch MyApplicationException not having to worry that they would catch an 'out of memory'-exception. But I agree, what do we do if a new critical exception appears?
|
|
|
|
|
As a former VB developer, I agree with your analysis. I can say that I see where he is coming from. It is a misguided way of thinking that comes from his VB experience. In VB there is no concept of exception handling. Coming from a VB background one will think that all errors are bad, when in fact when they are not. I mean really if ANY error occurs it must be trapped by a ON ERROR GOTO statement, which jumps to a block of code that handles ALL errors, and it is optional to check what ERR is set to so that you can get an integer value of the last error. This "flat" approach to error handling is what he is attempting to partially recreate when it's a horrible system and bad design from the start.
I used to write code that would let no error pass by, and I thought I was writing "solid" code. That just wasn't true. The fact is, if something "unexpected" happens then you need to let it follow through it's due course, and not try to handle everything just to keep something from crashing. Crashing is meant to allow a program has malfunctioned to cease operating. Interfering in this often causes more harm than good. Exceptions are GOOD things, not BAD things. Embrace them!
|
|
|
|
|