Is there a preference for nested try/catch blocks?
Asked Answered
H

11

41

One of the things that always bugs me about using Readers and Streams in Java is that the close() method can throw an exception. Since it's a good idea to put the close method in a finally block, that necessitates a bit of an awkward situation. I usually use this construction:

FileReader fr = new FileReader("SomeFile.txt");
try {
    try {
        fr.read();
    } finally {
        fr.close();
    }
} catch(Exception e) {
    // Do exception handling
}

But I've also seen this construction:

FileReader fr = new FileReader("SomeFile.txt");
try {
    fr.read() 
} catch (Exception e) {
    // Do exception handling
} finally {
    try {
        fr.close();
    } catch (Exception e) {
        // Do exception handling
    }
}

I prefer the first construction because there's only one catch block and it just seems more elegant. Is there a reason to actually prefer the second or an alternate construction?

UPDATE: Would it make a difference if I pointed out that both read and close only throw IOExceptions? So it seems likely to me that, if read fails, close will fail for the same reason.

Hourglass answered 8/10, 2008 at 15:53 Comment(5)
The worst thing is actually that the close() method can throw an exception. If an exception happens, then what else could you do other than closing the stream? And then closing it throws another exception, so you should close the stream, which would... I don't like Java for these silly things.Glottology
@Glottology - What happens if the user is working on a set of streams and one fails to close? This could mean that all the other streams are in an unusable state and should be tested. If ignored then they might be in an unusable state and the user continues to do work and not be able to save.Searchlight
@Rontologist: My point is that you can't do anything but close it when an exception is thrown, so why throw WHEN closing? IMO closing should not fail, because you can't do anything about it and you cannot get out of it. The stream is bad after close, failed or not. In .NET, Close() does not throw.Glottology
@OregonGhost: If you are using a single stream that is valid. However when multiple streams are in play it is an excellent opportunity to trigger a sanity check the other streams to determine if user intervention is required.Searchlight
@Rontologist: I've never seen code that does that though. Most either shut-down when one stream fails (because you can't continue, even if the other streams are fine), or they continue and get an exception from the other streams when they are bad.Glottology
W
7

I would always go for the first example.

If close were to throw an exception (in practice that will never happen for a FileReader), wouldn't the standard way of handling that be to throw an exception appropriate to the caller? The close exception almost certainly trumps any problem you had using the resource. The second method is probably more appropriate if your idea of exception handling is to call System.err.println.

There is an issue of how far exceptions should be thrown. ThreadDeath should always be rethrown, but any exception within finally would stop that. Similarly Error should throw further than RuntimeException and RuntimeException further than checked exceptions. If you really wanted to you could write code to follow these rules, and then abstract it with the "execute around" idiom.

Wagram answered 8/10, 2008 at 16:31 Comment(0)
V
26

I'm afraid there's a big problem with the first example, which is that if an exception happens on or after the read, the finally block executes. So far so good. But what if the fr.close() then causes another exception to be thrown? This will "trump" the first exception (a bit like putting return in a finally block) and you will lose all information about what actually caused the problem to begin with.

Your finally block should use:

IOUtil.closeSilently(fr);

where this utility method just does:

public static void closeSilently(Closeable c) {
    try { c.close(); } catch (Exception e) {} 
} 
Viable answered 8/10, 2008 at 16:9 Comment(10)
Beat me to it. One note: If you are using a logging subsystem, I would include a log message in the closeSilently if an exception occurs with a note that it was supressed. Doing nothing in response to the exception can hide essential diagnostic information later on.Immure
Very true. Perhaps omitting the log statement was a factor in me beating you to the answer?Viable
Swallowing an exception like that can be dangerous. At the very least you should be logging the stacktrace in the catch. Wrapping the exception in a runtime exception might be a better way to do it depending on the circumstances.Searchlight
@Rontologist: But the point is that we don't want to throw any exceptions from the finally block, so a runtime exception would be just as bad.Pifer
That's why I said it should be logged first. ;) However I would consider a simple swallow a bad practice. If the closeable is a file and we failed to close it we should probably let the user know. Same if it is a network connection. They both indicate a bigger problem might be occurring.Searchlight
I do not like this solution. What if close throws an exception, but read does not? No exception is passed to the caller and no error handling occurs. Maybe not so bad with readers, but what about writers?Atbara
McDowell's point about writers is, I think, an important one. It is very likely that close may call flush, so it's very possible that closing a stream with bytes remaining to be written can throw an exceptions, e.g. if the disk is full.Hourglass
Jakarta Commons IO have something like that.Ortega
It seems like the inevitable result of this problem is always some kind of wrapper class where the exception is retrieved through a getter. I've seen a bunch of variations of that idea, but have never been able to come up with something better or more elegant. Maybe "elegant" isn't reasonable.Immure
To avoid loosing the information from the first exception, I believe you can chain them together: download.oracle.com/javase/tutorial/essential/exceptions/…Usury
W
7

I would always go for the first example.

If close were to throw an exception (in practice that will never happen for a FileReader), wouldn't the standard way of handling that be to throw an exception appropriate to the caller? The close exception almost certainly trumps any problem you had using the resource. The second method is probably more appropriate if your idea of exception handling is to call System.err.println.

There is an issue of how far exceptions should be thrown. ThreadDeath should always be rethrown, but any exception within finally would stop that. Similarly Error should throw further than RuntimeException and RuntimeException further than checked exceptions. If you really wanted to you could write code to follow these rules, and then abstract it with the "execute around" idiom.

Wagram answered 8/10, 2008 at 16:31 Comment(0)
P
4

I prefer the second one. Why? If both read() and close() throw exceptions, one of them could be lost. In the first construction, the exception from close() overrides the exception from read(), while in the second one, the exception from close() is handled separately.


As of Java 7, the try-with-resources construct makes this much simpler. To read without caring about exceptions:

try (FileReader fr = new FileReader("SomeFile.txt")) {
    fr.read();
    // no need to close since the try-with-resources statement closes it automatically
}

With exception handling:

try (FileReader fr = new FileReader("SomeFile.txt")) {
    fr.read();
    // no need to close since the try-with-resources statement closes it automatically
} catch (IOException e) {
    // Do exception handling
    log(e);
    // If this catch block is run, the FileReader has already been closed.
    // The exception could have come from either read() or close();
    // if both threw exceptions (or if multiple resources were used and had to be closed)
    // then only one exception is thrown and the others are suppressed
    // but can still be retrieved:
    Throwable[] suppressed = e.getSuppressed(); // can be an empty array
    for (Throwable t : suppressed) {
        log(suppressed[t]);
    }
}

Only one try-catch is needed and all exceptions can be safely handled. You can still add a finally block if you like, but there is no need to close the resources.

Pifer answered 8/10, 2008 at 16:10 Comment(0)
A
2

If both read and close throw an exception, the exception from read will be hidden in option 1. So, the second option does more error handling.

However, in most cases, the first option will still be preferred.

  1. In many cases, you can't deal with exceptions in the method they are generated, but you still must encapsulate the stream handling within that operation.
  2. Try adding a writer to the code and see how verbose the second approach gets.

If you need to pass all the generated exceptions, it can be done.

Atbara answered 8/10, 2008 at 16:37 Comment(0)
P
1

The difference, as far as I can see, is that there are different exceptions and causes in play on different levels, and the

catch (Exception e)

obscures that. The only point of the multiple levels is to distinguish your exceptions, and what you'll do about them:

try
{
  try{
   ...
  }
   catch(IOException e)
  {
  ..
  }
}
catch(Exception e)
{
  // we could read, but now something else is broken 
  ...
}
Polyptych answered 8/10, 2008 at 15:58 Comment(0)
O
1

I usually do the following. First, define a template-method based class to deal with the try/catch mess

import java.io.Closeable;
import java.io.IOException;
import java.util.LinkedList;
import java.util.List;

public abstract class AutoFileCloser {
    private static final Closeable NEW_FILE = new Closeable() {
        public void close() throws IOException {
            // do nothing
        }
    };

    // the core action code that the implementer wants to run
    protected abstract void doWork() throws Throwable;

    // track a list of closeable thingies to close when finished
    private List<Closeable> closeables_ = new LinkedList<Closeable>();

    // mark a new file
    protected void newFile() {
        closeables_.add(0, NEW_FILE);
    }

    // give the implementer a way to track things to close
    // assumes this is called in order for nested closeables,
    // inner-most to outer-most
    protected void watch(Closeable closeable) {
        closeables_.add(0, closeable);
    }

    public AutoFileCloser() {
        // a variable to track a "meaningful" exception, in case
        // a close() throws an exception
        Throwable pending = null;

        try {
            doWork(); // do the real work

        } catch (Throwable throwable) {
            pending = throwable;

        } finally {
            // close the watched streams
            boolean skip = false;
            for (Closeable closeable : closeables_) {
                if (closeable == NEW_FILE) {
                    skip = false;
                } else  if (!skip && closeable != null) {
                    try {
                        closeable.close();
                        // don't try to re-close nested closeables
                        skip = true;
                    } catch (Throwable throwable) {
                        if (pending == null) {
                            pending = throwable;
                        }
                    }
                }
            }

            // if we had a pending exception, rethrow it
            // this is necessary b/c the close can throw an
            // exception, which would remove the pending
            // status of any exception thrown in the try block
            if (pending != null) {
                if (pending instanceof RuntimeException) {
                    throw (RuntimeException) pending;
                } else {
                    throw new RuntimeException(pending);
                }
            }
        }
    }
}

Note the "pending" exception -- this takes care of the case where an exception thrown during close would mask an exception we might really care about.

The finally tries to close from the outside of any decorated stream first, so if you had a BufferedWriter wrapping a FileWriter, we try to close the BuffereredWriter first, and if that fails, still try to close the FileWriter itself.

You can use the above class as follows:

try {
    // ...

    new AutoFileCloser() {
        @Override protected void doWork() throws Throwable {
            // declare variables for the readers and "watch" them
            FileReader fileReader = null;
            BufferedReader bufferedReader = null;
            watch(fileReader = new FileReader("somefile"));
            watch(bufferedReader = new BufferedReader(fileReader));

            // ... do something with bufferedReader

            // if you need more than one reader or writer
            newFile(); // puts a flag in the 
            FileWriter fileWriter = null;
            BufferedWriter bufferedWriter = null;
            watch(fileWriter = new FileWriter("someOtherFile"));
            watch(bufferedWriter = new BufferedWriter(fileWriter));

            // ... do something with bufferedWriter
        }
    };

    // .. other logic, maybe more AutoFileClosers

} catch (RuntimeException e) {
    // report or log the exception
}

Using this approach you never have to worry about the try/catch/finally to deal with closing files again.

If this is too heavy for your use, at least think about following the try/catch and the "pending" variable approach it uses.

Ourselves answered 8/10, 2008 at 16:56 Comment(0)
M
0

The standard convention I use is that you must not let exceptions escape a finally block.

This is because if an exception is already propagating the exception thrown out of the finally block will trump the original exception (and thus be lost).

In 99% of cases this is not what you want as the original exception is probably the source of your problem (any secondary exceptions may be side effects from the first but will obscure your ability to find the source of the original exception and thus the real problem).

So your basic code should look like this:

try
{
    // Code
}
// Exception handling
finally
{
    // Exception handling that is garanteed not to throw.
    try
    {
         // Exception handling that may throw.
    }
    // Optional Exception handling that should not throw
    finally()
    {}
}
Meperidine answered 8/10, 2008 at 16:27 Comment(1)
What if a ThreadDeath arrives while you're inside the finally block?Sulemasulf
G
0

2nd approach.

Otherwise, I don't see you catching the exception from the FileReader constructor

http://java.sun.com/j2se/1.5.0/docs/api/java/io/FileReader.html#FileReader(java.lang.String)

public FileReader(String fileName) throws FileNotFoundException

So, I usually have the constructor inside the try block as well. the finally block checks to see if the reader is NOT null before trying to do a close.

The same pattern goes for Datasource, Connection, Statement, ResultSet.

Garret answered 8/10, 2008 at 17:58 Comment(1)
The constructor in the first can be moved into the outer try block without issue. Possibly the outer try block is in a different method. Of course, the underlying stream is not closed in FileRead opened and then threw an unchecked exception before returning from the constructor.Wagram
I
0

Sometimes nested try-catch is not a preference, consider this:

try{
 string s = File.Open("myfile").ReadToEnd(); // my file has a bunch of numbers
 // I want to get a total of the numbers 
 int total = 0;
 foreach(string line in s.split("\r\n")){
   try{ 
     total += int.Parse(line); 
   } catch{}
 }
catch{}

This is probably a bad example, but there are times you will need nested try-cactch.

Inimical answered 8/10, 2008 at 19:52 Comment(0)
A
0

I like the approach by @Chris Marshall, but I never like to see exceptions getting swallowed silently. I think its best to log exceptions, especially if you are contiuing regardless.

I always use a utility class to handle these sort of common exceptions, but I would make this tiny different to his answer.

I would always use a logger (log4j for me) to log errors etc.

IOUtil.close(fr);

A slight modification to the utility method:

public static void close(Closeable c) {
    try {
      c.close();
    } catch (Exception e) {
      logger.error("An error occurred while closing. Continuing regardless", e); 
    } 
}
Aalii answered 8/10, 2008 at 23:43 Comment(0)
E
0

In some cases a nested Try-Catch is unavoidable. For instance when the error recovery code itself can throw and exception. But in order to improve the readability of the code you can always extract the nested block into a method of its own. Check out this blog post for more examples on nested Try-Catch-Finally blocks.

Emie answered 15/4, 2015 at 0:4 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.