6

From Effective Java 2e by Joshua Bloch,

An example of the sort of situation where it might be appropriate to ignore an exception is when closing a FileInputStream. You haven’t changed the state of the file, so there’s no need to perform any recovery action, and you’ve already read the information that you need from the file, so there’s no reason to abort the operation in progress. Even in this case, it is wise to log the exception, so that you can investigate the matter if these exceptions happen often.

Is it wise to always ignore that exception, as suggested. The code then would look like,

FileInputStream stream = null;
try {
    stream = new FileInputStream("foo.txt");
    // do stuff
} finally {
    if (null != stream) {
        try {
            stream.close();
        } catch (IOException e) {
            LOG.error("Error closing file. Ignoring this exception.", e);
        }
    }

which is much more verbose than

try (FileInputStream stream = new FileInputStream("foo.txt")) {
    // do stuff
}

But is the extra verbosity worth ignoring the exception from closing the file? Is it better to ignore the exception since there's nothing to be done? Should the same approach be used with resources other than a file, such as a database connection?

4
  • Possible duplicate of Is it ever ok to have an empty catch statement?
    – gnat
    Dec 30, 2017 at 11:57
  • 4
    Is it worth it? That depends, are you scheduling dentist appointments, or controlling a space probe, or a vending machine, or what? Dec 30, 2017 at 23:08
  • try { try(FileInputStream ....) { /* do stuff */ } } catch (IOException e) { ... } is much less verbose than your proposal. You don't get to differentiate between an error on closing and an error during reading, is the catch: but ask yourself -- what could cause an error during closing that wouldn't also cause one while reading?
    – Jules
    Mar 5, 2018 at 22:02
  • 1
    Alternatively, you could wrap your input streams in a class that extends FilterInputStream and catches any exception occurring during close, which is also much less verbose than your example code, and doesn't have the disadvantage of losing the ability to distinguish important and unimportant errors.
    – Jules
    Mar 5, 2018 at 22:05

8 Answers 8

4

The reason, which as I recall Bloch explains, is that the interesting Exception is the original one thrown from within the try block. Any Exception thrown within the finally block will hide the original Exception.

Imagine that the ur-Exception is because the File is on a network and the network is down. That's the more informative Exception that you want to throw. But that problem also affects the close() , and you don't want the 2nd exception to "get in the way".

Note this explanation was in V1 of his book...

Added The following code proves the point:

try {
  throw new Exception("in try, here is the ur-Exception");
}
finally {
  throw new Exception("in finally");
}

The second, "in finally" Exception, is the one you will see, hiding the original Exception.

2

For argument’s sake, I can’t imagine how this operation could fail, and what could go wrong if it fails.

Therefore, I would do nothing except logging in production. And make sure that the app runs into a breakpoint when running on a developers machine. If this ever fails, I want to know about it, and why it happened, so I can figure out more correctly how to handle it.

3
  • If this throws either you lost the drive (think network drive) or something is seriously wrong. I would be inclined to log as unless explained it's a sign of a serious problem. Jan 1, 2018 at 1:29
  • For arguments sake, I don't know this. That's why I want it to run into a breakpoint, and have a chance to figure out what happened, and then figure out how to handle the situation.
    – gnasher729
    Jan 1, 2018 at 1:54
  • Yeah, I would put an assertion in the catch--make sure it blows in debug mode. Jan 1, 2018 at 5:20
1

In general you should expect your program to run without ever throwing an exception.

Therefore, when an exception is thrown, its unusual that you would want to ignore it.

However, it's very usual that you don't want your program to crash. So people put top level try catch do nothing blocks to stop stuff they haven't thought of from crashing their applications completely.

In this case though really you do want to log the exception you hadn't thought of, so that you can update your code to deal with it properly.

In the FileInputStream example, yes it's not really clear why this would ever throw an exception (perhaps the file has been deleted?) or what you would do about it if it did.

Potentially I suppose if the close fails to complete successfully you might have a memory leak. So if its happening a lot you will want to know about it.

12
  • 1
    "In general you should expect your program to run without ever throwing an exception." - that's not a good guideline, platform and language choices can make throwing and catching exceptions regularly in normal operation of a program. Dec 30, 2017 at 23:11
  • 1
    Actually I do want my program to crash if it's going into some weird undefined state that might format my hard drive, corrupt the office database, or send the president threatening emails. If there is no reliable way to recover, rolling over and dying is the desired behavior. Much better than undefined behavior. Dec 31, 2017 at 0:33
  • @candiedorange maybe. but even in those cases you would want to control the error message or code thrown
    – Ewan
    Dec 31, 2017 at 8:41
  • 1
    Except checking first causes a race condition, you have to be able to handle the exception anyway. So you might as well just open without checking and deal with the exception. Dec 31, 2017 at 18:25
  • 1
    Worrying about exception handling performance when opening a file is premature optimization. And the file handling libraries do distinguish the differences, throwing different exceptions reflecting different problems. Dec 31, 2017 at 18:36
1

then and now, very sometimes. Yes. But always? By God - never!

First note: you should be careful about over-generalizing a rule from a precise example.

Compare

void close(Closable target) {
    try {
        target.close();
    } catch (IOException e) {
        ???
    }
}

We don't have nearly enough information to dismiss this exception as unimportant. There are many exceptional situations which prevent a resource from closing successfully.

void close(FileInputStream target) {
    try {
        target.close();
    } catch (IOException e) {
        ???
    }
}

Here, we have the additional context that we are reading data (no worries about being corrupted), and that the caller had already obtained the expected data (the caller is invoking close()). In other words, although we are in an exceptional situation, the circumstances don't indicate that the caller will be unable to ensure its explicit postcondition.

We shouldn't confuse a case where we accrue business value but leak a file handle with a case where there are indications of actual data loss.

I personally wouldn't get too hung up on verbosity; you can use another Closable to do the work

try( ClosePolicy closer = new LoggingClosePolicy(new FileInputStream(...))) {
    FileInputStream stream = closer.get();
    // ...
}

which I think is a bit better on the intention revealing scale: the default strategy for exceptions during close isn't satisfactory, so we are replacing it.

With that debris cleared, I think you can then start to address your original question: should we always...? It's a checked exception, so we cannot passively ignore the possibility.

I see two possibilities; if the local method cannot establish its postcondition in the face of the exception, then I think you need to propagate it in some form.

But I think it's much more common that the local method will be able to establish the observable portions of the post condition, in which case log-and-move-on seems like a reasonable way to accrue business value when the exceptional circumstances don't require immediate remediation.

1
  • 1
    I don't regard your distinction between close(Closable target) and close(FileInputStream target) valid. A method named "close" has one single responsibility, to close the target. So if it can't do that, it didn't fulfill its contract and really should signal its failure. If the method is e.g. "readJpegImage()", then failure when closing the stream can still mean success of reading the image, so no exception is necessary. Jan 1, 2018 at 12:55
1

Note that this problem is not limited to the Java language, or for that matter languages that have exceptions. You can face exactly the same problem when programming in C. Do you check the return value from fclose()? Or from close()?

I have seen both approaches: check and not check. Personally, I try to code finalizers so that they don't return an error code, just silently ignoring all errors. In some cases when I'm interested about catching bugs, I fail early in a big way, by calling abort() if I see that some important precondition is violated. The core dump left behind explains everything. Needless to say, in the final program the abort() function is never called. Don't use abort() if e.g. a network connection or some external resource fails; it's only for preconditions!

In the case of files, you probably won't fail closing it. But however, in the case of some network streams, there may be perhaps an error condition.

Also, you could consider logging the exception and then ignoring it silently. If you see lots of such exceptions in the logs, you can turn off logging for the biggest log size offenders.

1

To me there's one simple rule about exceptions:

If a method doesn't fulfill its contract, it throws an exception.

Now there are explicit and implicit aspects of a method's contract. The explicit contract typically (hopefully) correlates with its name. Implicit contract parts can be leaving the file system in the same state as before the method call, not creating resource leaks, and so on. It's a design decision, which implicit duties a given method has.

What's the contract of your method?

  • E.g. to read some data from a configuration file.

What's the effect of a failure when closing the stream?

  • The file has been read successfully, so the explicit contract has been fulfilled.
  • Failing to close a FileInputStream results in some resource leak. For a single configuration file read once in the application's run-time, that's hardly a problem, so typically doesn't violate the implicit contract.
  • Not closing a FileInputStream might (depending on the environment: OS, file system, ...) leave a lock on the file, inhibiting later modification by some other process. As configuration files typically aren't modified often (and in our case need an application restart when modified), that also is acceptable behaviour of our read method.

So yes, in this case of reading a single configuration file, I'd log the exception with a WARN level and return without an exception.

But if the method were the fileCopy() function of a file manager application, then not closing the source file with the risk to leave a lock on the file would definitely violate the implicit contract, so needing an exception.

1

The 3rd edition of Effective Java contains a new section with the title "Prefer try-with-resources to try-finally". In it, Bloch writes, "Always use try-with-resources in preference to try-finally when working with resources that must be closed," and provides a rationale which includes the 3 following points:

  • An exception in the finally block will hide the exception in the try block, exactly as user949300 describes in his answer.

  • try-finally is difficult to read when used with more than one resource.

  • try-finally is often used incorrectly. "In fact, two-thirds of the uses of the close method in the Java libraries were wrong in 2007."

So to answer the question, Bloch seems to think that try-with-resources is worth it.

2
  • Note that all answers except 1 actually assume that try-with-resources does not exist, since that was the case during the writing of the book, and instead concentrate on either "Is it OK to ignore an exception?" or "Why would you ignore an exception when suppressed isn't available?". You shouldn't translate advice of a 10 year old book on 7 year old technology.
    – Ordous
    Mar 5, 2018 at 20:24
  • I included a try-with-resources snippet in my original question because I was positing that as an alternative. Perhaps I should have explicitly stated I was looking for opinions on whether to use try-with-resources or try-catch and ignore the exception on closing the resource.
    – Max
    Mar 5, 2018 at 21:14
-1

Bloch proposes to swipe the garbage under the rug and hide it from the user by pretending it is not there, which I am certain an honest programmer (or program) will never do. I should make the program fail and crash real hard in on order to detect all possible errors at their first occurrences. Error recovery is another matter, though.

There also seems to be a redundancy in your code—If you create the stream outside the try block, you will not have to check whether it is null in the finally because it will always have been created.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.