Could someone explain to me why it is considered inapropriate to have a try-catch in the main() method to catch any unhandled exceptions?
[STAThread]
static void Main()
{
try
{
Application.Run(new Form1());
}
catch (Exception e)
{
MessageBox.Show("General error: " + e.ToString());
}
}
I have the understanding that this is bad practice, but not sure why.
You've got the catch-all exception there which will trap everything. So if you've got any unhandled exceptions in you code you'll never see them.
On the positive side, your application will never crash!
If this is required behaviour then you'll need to have sufficient logging and reporting to let both the user and you, as developer, know what's happened and recover or exit as gracefully as possible.
I don't see how that's bad practice at all.
Letting your program crash with an unhandled exception error isn't going to instill any confidence in your end users.
Maybe someone else could provide a counter view.
Update: Obviously you'll need to do something useful with the exception.
I don't think that's a bad practice in and of itself. I think the bad practice would be if that was the ONLY try/catch block you had in your application.
I don't think its necessarily bad practice. There are a few caveats however...
I believe the point of whoever called this "bad practice" was to reinforce the idea that you should be catching exceptions closest to where they occur (i.e. as high up the call stack as possible/appropiate). A blanket exception handler isn't typically a good idea because its drastically reduces the control flow available to you. Coarse-grained exception handling is quite importantly not a reasonable solution to program stability. Unfortunately, many beginner developers think that it is, and take such approaches as this blanket try-catch statement.
Saying this, if you have utilised exception handling properly (in a fine-grained and task-specific manner) in the rest of your program, and handled the errors accordingly there (rather than juist displaying a generic error box), then a general try-catch for all exceptions in the Main method is probably a useful thing to have. One point to note here is that if you're reproducably getting bugs caught in this Main try-catch, then you either have a bug or something is wrong with your localised exception handling.
The primary usage of this try-catch with Main would be purely to prevent your program from crashing in very unusual circumstances, and should do hardly any more than display a (vaguely) user-friendly "fatal error" message to the user, as well as possibly logging the error somewhere and/or submitting a bug report. So to conclude: this method does have its uses, but it must be done with great care, and not for the wrong reasons.
I'm not sure I think its a bad practice. What you want to do is make sure that the exception and the current state of the program when it crashes ends up in the hands of a developer, preferably logged with date, time and the user who was working with it. Basically - you want to make sure that your team has all the information they need to debug the problem, regardless of whether or not the user goes to them about the crash. Remember that many users will not, in fact, contact support if they get a crash.
The bad practice here would be catching the exception, showing a simple "Error" dialog box, and closing the application. In that case, the state of that exception is lost forever.
Well, this method will only capture exceptions thrown in your main thread. If you use both the Application.ThreadException and the AppDomian.UnhandledException events instead then you'll be able to catch and log all exceptions.
From a debugging standpoint, this can make life more difficult, as it makes every exception a user handled exception. This changes the debugger's behavior, unless you break on unhandled exceptions, which potentially has other issues.
That being said, I think this is a good practice at release time. In addition, I recommend listening on the AppDomain.UnhandledException and the Application.ThreadException events, as well. This will let you trap even more exceptions (such as some system exceptions that will not be caught in your "global" handler above).
That allows you to log the errors and provide the user with a good, clean message.
I'm not saying it's bad practice, the only thing I would do different though is use the built in event for that:
Application.ThreadException += new System.Threading.ThreadExceptionEventHandler(Application_ThreadException);
static void Application_ThreadException(object sender, System.Threading.ThreadExceptionEventArgs e)
{
MessageBox.Show(e.Exception.Message); //or do whatever...
}
Change it to this and it's fine
catch(Exception ex)
{
YourLoggingSystem.LogException(ex);
}
Of course, this line should NEVER be hit as you'll have other exception handlers throughout your code catching things with much more context.
Top-level exception handling is pretty essential, but I'd recommend using:
Application.ThreadException += new ThreadExceptionEventHandler(YourExceptionHandlingMethod);
However, this will only catch exceptions on the GUI thread (much like your try..catch block) - you should use similar code for each new thread you start to handle any unexpected exceptions from them.
More about it here.
I think it is a best practice to have try..catch in your main().
However, I would make a few modifications to your code (to catch the exception log it, and then message box:
[STAThread]
static void Main()
{
try
{
Application.Run(new Form1());
}
catch (Exception ex)
{
EventLog log;
log = new EventLog("MyApp", Environment.MachineName, "Application");
// write to the event log
log.WriteEntry(ex.message, Error);
MessageBox.Show("General error");
}
}
Any exception which gets to Main() is likely fatal.
If it was something easy, it should have been handled higher up. If it was something beyond your control, like OutOfMemoryException, then the program should crash.
Windows application which crash have a standard way of doing so, they trigger the Windows Error Reporting dialog. (You've likely seen it before). You can sign up to recieve crash data when this happens.
In antiquity, placing a try/catch in C++ caused a fairly heavy performance penalty, and placing one around main would mean storing extra stack info for everything, which again was bad for performance.
Now computers are faster, programmers less addicted to performance, and runtimes are better built, so it's not really bad anymore (but still you might pay a little more for it, haven't benchmarked it's effect in years). So it's old folklore like iterating against the grain (compilers actually fix the iteration anyways for you nowadays). In C# it's perfectly fine, but it'd look iffy to someone from 10 years ago.
I think this is discouraged because exceptions are caught only once and there may be multiple things, like background threads, that need to know if a process throws.
Is this a WinForms app? Forms.Application.Run raises events whenever a thread throws an unhandled exception. The MSDN docs try to explain this, but the handler they show doesn't work! Read the updated example from the WinForms UE site.
If your program tries to keep running despite a catch-all catching who-knows-what kind of violation of assumptions down below, it's in a danger zone for things like remote exploitation of security flaws (buffer overflows, heap corruption). Putting it in a loop and continuing to run is a big red flag for software quality.
Getting out as quickly as possible is the best, e.g. exit(1). Log and exit is good though slightly riskier.
Don't believe me? The Mitre Common Weakness Enumeration (CWE) agrees. Closely related is its advice against catching NULL pointer dereferencing in this way.
I could go into a lengthy explanation, but you're better off just reading FxCop team blog response.