Is it okay that I sometimes sink my exceptions?

Question

I have a best practices question. I realize this is subjective but wanted to ask people smarter than me if this is a common programming practice.

If you have a method of something NON-CRITICAL that you don't want to interfere with the important functioning of your application, is it common to use an error sink like this?

Try 
    'do stuff.  not important if it fails.

Catch ex as exception
    'sink.  do nothing.
End Try

If you were thinking of hiring me and you were reading some of my code and saw this...would you?

Seth

EDIT Wow! Thanks for your answers. I think the consensus is that should never be done or it should be very rare.

I thought I would give you context for this question. First, I have a strong familiarity with the Karl Sequin article and have followed that pattern for years.

But today on the project I was working on, I was working through the change list and was faced with the addition of a simple feature. (If you care to know...it is adding context menu support to a Rich Text Box.)

The attached note said, "if it takes longer than 15 mins...drop it."

So I am faced with adding what is a potentially useful feature but the don't really have the time to test that it won't break working features. For the record, our exception handler for this system DOES have a mechanism for handling and sinking or logging these errors. But what if I was working on a system that did not have a robust error handling system. Would it be okay to add this feature and if an error occurs...nothing is really lost.

That was my thinking. But I have taken your message to heart...that basically this is a bad idea.

Seth

Answer 1

Yes, it is common, but in general it shouldn't be done.

There are exceptions like OutOfMemoryException which are better not caught, unless you catch them to attempt to terminate your application gracefully.

In the majority of cases, swallowing System.Exception or System.SystemException will inevitably hide further run-time problems.

Answer 2

It's common, but it's best to avoid it. At the very least you should log the exception somewhere.

Also, avoid using exceptions for flow control. Your code should be written to handle all likely cases without resorting to exceptions. Aside from performance increases, this method also tells you that when an exception happens, it's worth your attention.

Answer 3

You shouldn't be catching all exceptions here. It may, on a rare occasion, be okay to ignore a particular, expected exception. However, catching all exceptions and swallowing them will mean that you try to continue after a OutOfMemoryException and other irrecoverable exceptions

Answer 4

Best practices says that you shouldn' do this. What is so unimportant that you wouldn't want to know that an error occurred? I don't think I've ever encountered this.

I'd either not use the try catch structure or I refactor the code in to a method that returns a boolean so that I at least can identify if it succeeded or not...

That's my thought...

Update: There are times where you might need to sink an exception. In these cases, I'd identify which exceptions might be candidates for this and then catch those specific exception. But this is far from catching Exception which could be any thing, such as the aforementioned OutOfMemoryException. In short, I'd only catch specific exceptions with the intent to ignore it.

Answer 5

I would say "Don't do it" - not like that.

First of all, try refactoring the "noncritical" code so that it doesn't throw an exception.

If you are unable to do that, at the very least don't blindly catch Exception. Only catch the exceptions that you expect it to throw (and log them somewhere!) - anything else is something you need to be made aware of.

Answer 6

Personally I see this as being incredibly bad practice.

It is (unfortunately) one of the things I always look for when reviewing code, and the questions I ask when I find empty catch blocks or catch blocks that essentially swallow exceptions are:

1. At this moment in time are you 100% sure that this exception will never matter?
2. Are you also 100% certain that any exception caught here in the future will never matter, regardless of how the code base develops?
3. Even if 1 and two are both true, is it really so hard to put some logging here?

The most important thing for me is the logging - good logging of exceptions, and good tracing of program execution are fundamental to the design of code that can be safely modified over time, and that evolves into a stable system that users have faith in.

Beyond that, a good practise is to only catch specific exceptions, and to let all other exceptions bubble up the stack. Blindly handling exceptions as a way of handling errors is never correct.

Answer 7

.if using vs2008 you could at least send them to the debug window

 System.Diagnostics.Debug.WriteLine("exception in method - my method -: "+ex.message);

Answer 8

You should never hide Exception. I might hire you anyway, but you wouldn't do that while you worked for me :)

Seriously, though, in Java (for instance) Exceptions are used for certain cases that you may choose to ignore, like FileNotFoundException (like you did, with a comment). But if you ever catch a type of Exception -- like IOException -- you cannot just hide it. If the Exception means nothing to you in particular, wrap it in a RuntimeException and throw it up to the client.

Somewhere along the way, Exception should be handled, but never hidden.

Answer 9

No, that's not OK in my opinion.

Your system might not be critical, but presumably you do want it to run otherwise you wouldn't have written it in the first place. Now if one day it stops running, the people who have to maintain it have no idea why it's suddenly not working, as you're completely hiding the error. In fact it might even take them a long time to realize where the error is coming from or even that there is an error in the first place. It could waste a lot of developer time, whereas it wouldn't have taken much time to log the error instead of discarding it.

And you're catching exceptions like OutOfMemoryException that you really don't want to catch and discard. If that code is non-critical but is consuming far too much memory and making your whole system slow, you want to be told that so you can fix it.

Answer 10

Best Practices for Handling Exceptions (MSDN):

According to them you can either

Do something with the error or ignore it.

It's up to you and your team to decide I guess.

Answer 11

It's almost never a good idea, but depending on how you do exceptions there are cases when it's applicable. For example:

try {
   Cache.refresh(myObject)
}
catch (NotInCacheException) {
   // If the object isn't already in the cache, then we don't have to worry 
   // about refreshing it. It's assumed that this is a common occurrence,
   // so we don't need to log it either; that ends up just spamming
   // the logs. So, just swallow it, silently.
}

(Note: Whether or not Cache.refresh() should be throwing a NotInCacheException at all is not at issue here; I'm just using this as a quick example).

Caveat though: you can only get away with this if you have a very specific exception that you're looking for. In your example, you're catching Exception, which is way too high-level (you'll be swallowing critical errors as others have mentioned).

Also: If you have a flexible logging framework, you might want to log this exception anyway for informational purposes. I'm thinking of Log4Net/Log4J in particular here: you can configure your logging (during runtime even) to select/ignore exceptions based on class and severity. Thus, you could silence these types of exceptions under normal circumstances, but log them if you wanted to do some poking around your application.

If you do decide to swallow the exception, my policy is to always have a code comment explaining why you did so; this marks it as not just another bad dev practice.

Answer 12

You should make a distinction between ignorable exceptions and fatal exceptions. I'm not sure how this is done in VB, but in Java (for example), the exceptions derived from Error should never be ignored. This includes things like class loader failures, out of memory conditions, stack overflow, etc.

Answer 13

Sometimes I find that developers would rather catch and swallow an exception instead of writing correct error handling code.

There's some code, if throws an exception, they catch the exception, try something else, and if that throws an exception, do something else, etc. Exceptions are objects and are created when they are thrown. If you can, write the correct handling code to deal with an error and only use exceptions when you can't continue for whatever reason.

As for swallowing exceptions, I'd be lying if I said I'd written code which didn't. It's not a best practice and not to mention extremely hard to debug when your program behaves poorly. I've been bit by them too.

Answer 14

I do this only at annoying excpetions like closing a connection:

try {
   conn.close()
}
catch( Exception e ) {
}

because I'm already sure that I don't need it anymore.

Answer 15

My take is that you can do it, but you have to keep your eyes open and know why you're doing it. Think it through. In my shop we require that you have a comment explaining why you're eating the exception

Answer 16

While most people here are actually talking about the developer, I would like to point out another viewpoint of the story.

I admit: I have swallowed exceptions and in every case it was because IMHO the designer of the function screwed up.

"Exceptions" means "exceptions", not failure or error. What I mean is: If the function must acknowledge that the input may not be correct, I expect that the function does NOT use exceptions. My wrath targets particularly "ParseException"s.

If you parse input, you will very likely find unknown/corrupted/inexact input. That is "normal", not an exception. And in this case I find it annoying if the developer of the function throws ParseException if the function couldn't properly parse.

Why ?

• The exception breaks the control flow
• The exception is slow
• Often it doesn't matter anyway because you are initializing with default values.

If you call the parseFunction several thousands or millions(!) of times you are clogging up your log file for exactly nothing.

In contrast my ideal function gives back a class object with status code and message like

StatCode stat = parseXYZ(Reader r);

which can be then be tested.

if (stat.code != OK)  
  //whatever

If in the other way you are experiencing problems which you can't foresee (file not locked, nonsensical argument, illegal thread state) exceptions are excellent.

Answer 17

If this is a good thing or not depends on the language and how expensive exceptions are. You still shouldn't catch ALL exceptions, but, for example, in Python the following is a common idiom:

d={}
for i in theList:
  try:
    d[i] += 1
  except KeyError:
    d[1] = 1

And that's the fastest way to do default insertion. You'd do the same thing for a DivisionByZeroError, or any number of other exceptions. And the definition of a bunch of flow control constructs boils down to raise StopIteration

Answer 18

As a rookie assigned to doing bug fixes, I discovered that the particular bug I had been assigned was printing (to debug) "This should never happen." After getting over my fear of working in an error condition that apparently was impossible, I finally managed to discover that it was caused by a subtle race condition. If the author of that code originally had simply sunk the exception without at least that meager debug statement, it would have been much more difficult to track down the race condition.

Since then I've become a huge proponent of either dealing with exceptions or propegating them until the error condition is shown in a user friendly way to the end-user, a step futher from simply logging a debug statement.

Answer 19

I think the best way to answer this is to talk about cases where it IS acceptable to do it. In most cases it is NOT safe. In those cases where it is safe, it is usually best to at least spit out some information about it in the warn, debug, or trace log. I can think of only a few cases where it is safe:

You expect a very specific exception, and know that you can recover from it:

try {
 Integer.Parse(someString);
}
catch (ParseException pe) {
 //In this particular use case, if the string doesn't parse, 
 //I don't need to do anything. Logging a warning might be a
 //good idea, in some cases
}

You are making cleanup code safe:

beginDatabaseTransaction();
try {
 doSomeDatabaseOperation();
}
catch (Exception e) {
 log.error(e); //log the original exception
 try {
  rollbackDatabaseConnection();
 }
 catch(Exception e2) {
  //You may ignore this one-off exception, though
  //I would strongly consider logging it
 }
 throw; //rethrow the original exception and let it propagate
}

(This is by no means a best practice pattern for database operations; just a contrived example.)

In our production system, we have had more than one case where overzealous exception trapping caused much more damage than it did good. Be careful with this.

One other thing: If you don't except a specific error to be thrown, you usually should not be trying to catch one.

Most exceptions are there to help you, the developer, find and diagnose mistakes in your code. They are designed to make the program (or the current operation) terminate, to prevent unpredictable behavior (like trying to load all of the records in your the database, because you weren't able to construct the WHERE clause for your SQL statement.) When you catch an unknown exception, you are concealing the mistake from your currently-running code and making it possible for the code to behave unexpectedly, instead of just terminating.

Answer 20

Java brought to the fore the need to program with Exceptions. I've found Java tends to willingly throw Exceptions, which is annoying because you have to catch them if you don't want your app to crash. Unfortunately, Exceptions in Java are a fairly heavy feature.

I learnt about effectively using exceptions in Icon. Icon has this neat thing called 'Failure'. If an expression doesn't have a value it can return, it will instead Fail. This functions like a miniature exception only instead of the try {} catch {} notation, Failure is handled by language elements directly, such as if and while.

IMO, that is how Exceptions should work in Java... but unfortunately, they don't. Instead, you end up writing defensive code to prevent Exceptions rather than handling them easily and naturally. Silently swallowing a particular Exception because it's reasonably rare and much more elegant than preventing it I would regard as A Useful Trick. But it's the kind of thing that responds to "You must know the rules before you can break them". In other words, don't do it too often.

I wish more languages supported the Success/Failure model found in Icon.

Answer 21

Generally I'd say it's bad practice--but let's say the method in question was a logging method. I don't want my app to crash because the EventLog is full. In a case like that, I'd say it's okay. I'd still output it to the Debug window though.