Swallowing exception thrown in catch/finally block

Question

Usually I come across situations where I have to swallow an exception thrown by the clean up code in the catch/finally block to prevent the original exception being swallowed.

For example:

// Closing a file in Java
public void example1() throws IOException {
    boolean exceptionThrown = false;
    FileWriter out = new FileWriter(“test.txt”);
    try {
        out.write(“example”);
    } catch (IOException ex) {
        exceptionThrown = true;
        throw ex;
    } finally {
        try {
            out.close();
        } catch (IOException ex) {
            if (!exceptionThrown) throw ex;
            // Else, swallow the exception thrown by the close() method
            // to prevent the original being swallowed.
        }
    }
}

// Rolling back a transaction in .Net
public void example2() {
    using (SqlConnection connection = new SqlConnection(this.connectionString)) {
        SqlCommand command = connection.CreateCommand();
        SqlTransaction transaction = command.BeginTransaction();
        try {
            // Execute some database statements.
            transaction.Commit();
        } catch {
            try {
                transaction.Rollback();
            } catch {
                // Swallow the exception thrown by the Rollback() method
                // to prevent the original being swallowed.
            }
            throw;
        }
    }
}

Let's assumed that logging any of the exceptions is not an option in the scope of method block, but will be done by the code calling the example1() and example2() methods.

Is swallowing the exceptions thrown by close() and Rollback() methods a good idea? If not, what is a better way of handling the above situations so that the exceptions are not swallowed?

For resources that implement `IDisposable` (which is usually the case), I've [implemented an extension method](http://stackoverflow.com/a/35613744/1149773) that incorporates the logic of your `example1`, along with other strategies. — Douglas, Feb 24 '16 at 21:43

score 25 · Accepted Answer · answered Oct 31 '09 at 14:18

25

I'm not a fan of catching and rethrowing an exception.

If you catch it, do something with it - even if it's just logging the exception.

If you can't do anything with it, don't catch it - add a throws clause to the method signature.

Catching an exception tells me that either you can deal with an exceptional situation and have a recovery plan or "the buck stops here" because an exception cannot propagate in that form any farther (e.g., no stack traces back to the user).

answered Oct 31 '09 at 14:18

duffymo

305,152
44
369
561

2

Agree. Don't think of how to finish up the call - think of providing relevant information to the poor developer who has to fix an obscure and non-reproducable bug based only on the logs. – Thorbjørn Ravn Andersen Oct 31 '09 at 14:49
5

Disagree - sometimes "doing something with it" *is* rethrowing it - particularly catching a checked exception but passing a more meaningful unchecked exception to the client code (I believe this situation applies to Java only, however). Spring's JdbcTemplate is an excellent example of catching checked exceptions and exposing them as more meaningful unchecked exceptions. – MetroidFan2002 Oct 31 '09 at 16:07
@MetroidFan2002 - I think he meant catching and then rethrowing the same Exception object, not throwing a new one. – Ken Liu Oct 31 '09 at 16:41
1

@Metroid, I think you and Ken have summarized one of the cases that "the buck stops here" covers. If I catch and rethrow as a more meaningful and/or unchecked exception, I'm saying that the original exception cannot propagate outside that layer of the application. – duffymo Oct 31 '09 at 20:07
It also is okay to catch and rethrow when object state needs to be changed when an exception occurs. – Crono Nov 11 '14 at 19:12
What object state? Can't be a local one. The return state is the only thing that makes sense. If you can set it to a value, I'd say you don't have to rethrow - set the value. – duffymo Nov 11 '14 at 19:23
@duffymo It depends. If for some reason you want your object to remember how many attempts at doing something has failed, you'll want to catch, add 1 to the counting variable and then throw. – Crono Nov 17 '14 at 20:13

score 7 · Answer 2 · edited Apr 22 '20 at 10:59

7

You can create a custom Exception type that can hold both exceptions. If you overload the ToString(), you can log both exceptions.

try
{
    transaction.Commit();
}
catch(Exception initialException)
{
    try
    {
        transaction.Rollback();
    }
    catch(Exception rollbackException)
    {
        throw new RollbackException(initialException, rollbackException);
    }

    throw;
}

edited Apr 22 '20 at 10:59

d219

2,707
5
31
36

answered Oct 31 '09 at 14:24

CMerat

4,398
25
28

1

The point on the rollback is that something bad happened in the commit and you are handling it in with the rollback. One could argue the first exception is being handled by the Rollback and you don't need the initialException. If the rollback succeeds you never see the 1st exception. Its only when it fails do you see both. Anywho, I agree with the new exception type. If you want to see more than one exception, capture both and put into a new excetion. Great idea – shimpossible Oct 31 '09 at 14:30

fvu · Answer 3 · 2009-11-02T09:08:32.097

4

That's exactly why Commons IO has an IOUtils.closeQuietly method. In most cases what goes wrong during the closing of a file is not that interesting.

Database transactions that have to be rolled back are potentially more interesting, as in that case the function didn't do what it was supposed to do (put stuff in the DB).

edited Nov 02 '09 at 09:08

answered Oct 31 '09 at 14:18

fvu

32,488
6
61
79

score 1 · Answer 4 · answered Oct 31 '09 at 16:48

There's no reason to rollback the transaction in the C# code. If you close the connection wihtout rolling back the transaction (or committing it) that is equivalent and more efficient...

public void example2() {
  using (SqlConnection connection = new SqlConnection(this.connectionString))
  using (SqlCommand command = connection.CreateCommand())
  using (SqlTransaction transaction = command.BeginTransaction()) {
    // Execute some database statements.
    transaction.Commit();
  }
}

and you are done.

The using statement will ensure (via finally) that the connection is closed regardless of any exceptions and let the raw exception bubble out (with the full/correct stack trace). If an exception occurs before the call to Commit, the transaction will never commit and will be automatically rolled back when the transaction/connection closes.

score 0 · Answer 5 · answered Oct 31 '09 at 14:24

I believe that an exception should be something you don't expect. If you expect an exception then you should do something with it. So in your first example I think you should probably not bother catching the IOException if you are also stating that your method is going to throw IOException.

score 0 · Answer 6 · answered Oct 31 '09 at 14:47

I would consider rewriting example1 as follows:

// Closing a file in Java
public void example1() throws IOException {
    boolean success = false;
    FileWriter out = new FileWriter(“test.txt”);
    try {
        out.write(“example”);
        success = true;
        out.close();
    } finally {
        if (!success) {
            try {
                out.close();
            } catch (IOException ex) {
                // log and swallow.
            }
        }
    }
}

Moving the success = true; to after the out.close(); statement would make the meaning of success a bit clearer ... though it may result in out.close() being called twice.

score 0 · Answer 7 · answered Oct 31 '09 at 14:49

Without knowing all of what your particular circumstance is, you could look into throwing a new exception. In C#, at least, when throwing a new exception you one of the optional constructors accepts an existing exception as a parameter. For example:

throw new Exception("This is my new exception.", ex);

The purpose of this is to retain the original exception.

Another option might be the try .. catch .. finally construct.

try { // normal code that might throw an exception } catch (Exception ex) { // handle that first exception } finally { // handle any cleanup regardless of exception being thrown }

In general if my code can handle an exception in a particular try .. catch then I don't re-throw that exception. If it is important for something further up the call stack to have that exception I will throw a new exception and set the original exception as the inner exception.

score -2 · Answer 8 · answered Oct 31 '09 at 14:46

usually, risky codes are put all in one try-catch block. nested try-catch blocks are not a very good idea, IMO (or just try to avoid nested try-catch blocks as much as possible, unless you really need them).

because the risky codes are exceptional scenario, so putting exceptional codes for exceptional case for even more exceptional case, that's alot of unnecessary work.

for example in example1(), place all risky codes in one try-catch block:

try{
FileWriter out = new FileWriter(“test.txt”);
out.write(“example”);
out.close();
} catch(Exception e) {
    //handle exception
}

OR, another good idea is to place several catch(s) for the same try:

try{
    FileWriter out = new FileWriter(“test.txt”);
    out.write(“example”);
    out.close();
} catch(FileNotFoundException e) {
        //if IOException, do this..
} catch(IOException e) {
        //if FileNotFound, do this..
} catch(Exception e) {
        //completely general exception, do this..
}

You can leak file handles with this code. If `write` throws, `close` will not be called. — McDowell, Oct 31 '09 at 16:30

Swallowing exception thrown in catch/finally block

8 Answers8

Linked