Skip to end of metadata
Go to start of metadata

Methods invoked from within a finally block can throw an exception. Failure to catch and handle such exceptions results in the abrupt termination of the entire try block. Abrupt termination causes any exception thrown in the try block to be lost, preventing any possible recovery method from handling that specific problem. Additionally, the transfer of control associated with the exception may prevent execution of any expressions or statements that occur after the point in the finally block from which the exception is thrown. Consequently, programs must appropriately handle checked exceptions that are thrown from within a finally block.

Allowing checked exceptions to escape a finally block also violates ERR04-J. Do not complete abruptly from a finally block.

Noncompliant Code Example

This noncompliant code example contains a finally block that closes the reader object. The programmer incorrectly assumes that the statements in the finally block cannot throw exceptions and consequently fails to appropriately handle any exception that may arise.

public class Operation {
  public static void doOperation(String some_file) {
    // ... Code to check or set character encoding ...
    try {
      BufferedReader reader =
          new BufferedReader(new FileReader(some_file));
      try {
        // Do operations 
      } finally {
        reader.close();
        // ... Other cleanup code ...
      }
    } catch (IOException x) {
      // Forward to handler
    }
  }
}

The close() method can throw an IOException, which, if thrown, would prevent execution of any subsequent cleanup statements. This problem will not be diagnosed by the compiler because any IOException would be caught by the outer catch block. Also, an exception thrown from the close() operation can mask any exception that gets thrown during execution of the Do operations block, preventing proper recovery.

Compliant Solution (Handle Exceptions in finally Block)

This compliant solution encloses the close() method invocation in a try-catch block of its own within the finally block. Consequently, the potential IOException can be handled without allowing it to propagate further.

public class Operation {
  public static void doOperation(String some_file) {
    // ... Code to check or set character encoding ...
    try {
      BufferedReader reader =
          new BufferedReader(new FileReader(some_file));
      try {
        // Do operations 
      } finally {
        try {
          reader.close();
        } catch (IOException ie) {
          // Forward to handler
        }
        // ... Other cleanup code ...
      }
    } catch (IOException x) {
      // Forward to handler
    }
  }
}

Compliant Solution (try-with-resources)

Java SE 7 introduced a feature called try-with-resources that can close certain resources automatically in the event of an error. This compliant solution uses try-with-resources to properly close the file.

public class Operation {
  public static void doOperation(String some_file) {
    // ... Code to check or set character encoding ...
    try ( // try-with-resources
      BufferedReader reader =
          new BufferedReader(new FileReader(some_file))) {
      // Do operations
    } catch (IOException ex) {
      System.err.println("thrown exception: " + ex.toString());
      Throwable[] suppressed = ex.getSuppressed();
      for (int i = 0; i < suppressed.length; i++) {
        System.err.println("suppressed exception: " 
            + suppressed[i].toString());
      }
      // Forward to handler
    }
  }

  public static void main(String[] args) {
    if (args.length < 1) {
      System.out.println("Please supply a path as an argument");
      return;
    }
    doOperation(args[0]);
  }
}

When an IOException occurs in the try block of the doOperation() method, it is caught by the catch block and printed as the thrown exception. Exceptions that occur while creating the BufferedReader are included. When an IOException occurs while closing the reader, that exception is also caught by the catch block and printed as the thrown exception. If both the try block and closing the reader throw an IOException, the catch clause catches both exceptions and prints the try block exception as the thrown exception. The close exception is suppressed and printed as the suppressed exception. In all cases, the reader is safely closed.

Risk Assessment

Failure to handle an exception in a finally block may have unexpected results.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

ERR05-J

Low

Unlikely

Medium

P2

L3

Automated Detection

Tool
Version
Checker
Description
Coverity7.5PW.ABNORMAL_TERMINATION_ OF_FINALLY_BLOCKImplemented
Parasoft Jtest
10.3
PB.CUB.ARCF, PB.CUB.ATSFImplemented
SonarQube
6.7
S1163Exceptions should not be thrown in finally blocks

Related Guidelines

MITRE CWE

CWE-248, Uncaught Exception 

CWE-460, Improper Cleanup on Thrown Exception 

CWE-584, Return inside finally Block 

CWE-705, Incorrect Control Flow Scoping

CWE-754, Improper Check for Unusual or Exceptional Conditions 

Bibliography

[Bloch 2005]

Puzzle 41, "Field and Stream"

[Chess 2007]

Section 8.3, "Preventing Resource Leaks (Java)"

[Harold 1999]


[J2SE 2011]

The try-with-resources Statement



20 Comments

  1. This needs a second compliant solution using closeIgnoringException().

  2. rewrote the intro paragraph; please review for correctness.

  3. I'm a bit concerned that the compliant solution for Compliant Solution (Java 1.7: try-with-resources) is noncompliant with respect to ERR00-J. Do not suppress or ignore checked exceptions but still shown in blue. This violates are rule that all compliant examples must be compliant with all other rules.

    Is there an easy way to make the compliant solution compliant with this other rule? For example, I don't see where path is defined even though all of main is shown. Can the CS loop back to get a new path name?

    1. Addressed. The 'path' was a typo. The point of this code sample is to show how try-with-resources handles multiple exceptions, which is why the print statements are appropriate (in spite of ERR00-J).

      1. I think that argument is going in the wrong direction. That suggests also that if the point of your program is to demonstrate a buffer overflow, then a program with a buffer overflow is a compliant solution.

        My concern is that compliant solutions should be exemplars of secure programming, so that they can be copy/pasted into real systems.

        I made a small change that isn't ideal but perhaps is sufficient. This example would be helped if the printf statements were for debugging only (System.err?)

  4. I don't think there is anything wrong with throwing an exception from a finally block, whether checked or not. You really need to avoid closing more than one resource in a finally block. The path of execution should be as an exception in the unlikely event of an error disposing a resource.

    "try-with-resources" arranges that the innermost exception is thrown, but it will still throw checked exceptions from resource disposal.

    Output streams are somewhere tricky. FilterOutputStream.close flushes first. It will ignore the exception from the flush, which you don't want but cant be changed. So, I would suggest flushing the stream at the end of the try block, so that any exception in the otherwise happy case is noted. Also, the underlying resource can be closed rather than the decorator.

    The compliant code has a bit of a gap between the resource being allocated and the try block. Some people are fine with that, but I'd put the decorators within the try block. Also, FileReader implicitly picks up whatever character encoding happens to be configured, rather than the choice being explicit.

    A complication is the likes of ZipInputStream (I think), which in the Sun/Oracle implementation has resources of its own (C heap allocations) which should be disposed of.

    1. I don't think there is anything wrong with throwing an exception from a finally block, whether checked or not. You really need to avoid closing more than one resource in a finally block. The path of execution should be as an exception in the unlikely event of an error disposing a resource.

      "try-with-resources" arranges that the innermost exception is thrown, but it will still throw checked exceptions from resource disposal.

      The danger is having to deal with two exceptions being thrown from the same try/catch/finally statement. If an exception occurs in the try block, and another exception occurs in the finally block, the finally exception gets thrown, and the try exception gets forgotten. This occurs in both Java 1.6 and Java 1.7.

      The one exception (pardon the pun) to this fact is the try-with-resources construct in Java 1.7, where one exception gets thrown, but it provides access to the other exception via the getSuppressed() method. This method doesn't exist in earlier Javas. Also it is not used in normal try/catch/finally blocks; it is only used in try-with-resources.

      Also, we now permit exceptions that occur during a file close to be ignored, as per EX0 of ERR00-J. Do not suppress or ignore checked exceptions.

      BTW I killed the first NCCE, it violated ERR00-J and provided nothing useful.

      Output streams are somewhere tricky. FilterOutputStream.close flushes first. It will ignore the exception from the flush, which you don't want but cant be changed. So, I would suggest flushing the stream at the end of the try block, so that any exception in the otherwise happy case is noted. Also, the underlying resource can be closed rather than the decorator.

      The compliant code has a bit of a gap between the resource being allocated and the try block. Some people are fine with that, but I'd put the decorators within the try block. Also, FileReader implicitly picks up whatever character encoding happens to be configured, rather than the choice being explicit.

      Agreed. That 'throws IOException' was masking the fact that the file open occurs outside the try block. I moved it in all the code samples (except the try-with-resources, obviously (smile)

      A complication is the likes of ZipInputStream (I think), which in the Sun/Oracle implementation has resources of its own (C heap allocations) which should be disposed of.

    2. Also, FileReader implicitly picks up whatever character encoding happens to be configured, rather than the choice being explicit.

      Added a comment to each code sample noting the (elided) presence of code to check or set the character encoding. This should be a sufficient bow in the direction of IDS17-J.

  5. Suggestions:

    • main methods are not required
    • can make classes final
    • That last CS has main() that accepts args which is different from other CSs. The main is really not required to understand what's going on. Also, println statements can be avoided because other CSs do not have this functionality.

    More about the last CS:

    • This includes both any error while doing operations

      – what do you mean by error? Or do you mean subtype of IoException? s/error/exception?
    • When both the try block and also closing the reader throw an IOException, the catch clause catches both exceptions, and prints the try-block error as the thrown exception.

      – isn't this implicit? There is only one type of exception and the catch is also catching the single type. I also don't understand the latter part of the sentence (clarify whether the try exception overrides the reader-closing exception). What is the difference between a try "error" and exception because of closing the reader? Where are you closing the reader - wait this is automatic! Why not just say - any exceptions that happen while creating, closing or ... the stream are handled by the catch and there is no requirement of releasing resources explicitly in a finally block. Why not catch throwable instead of IoException. I thought that this guidelines is about closing resources and not about how to catch exceptions using this facility.
    • Specify that you can also declare multiple resources in the try etc.
  6. I also think it would be better to clarify that any exception in try that has the same type as any exception in the finally block will result in "exception swallowing". Such as read and write methods for IO also throw IOException and so does the IO stream classes (Bloch puzzle 41). Bloch also suggests using the Closeable interface (instead of the dedicated method that we have listed as 2nd CS):

    private static void closeIgnoringException(Closeable c) {
        if (c != null) {
            try {
                c.close();
            } catch (IOException ex) {
                // There is nothing we can do if close fails
            }
        }
    }
    

    This rule is not about abrupt termination of finally block but a reference to that rule will be useful.

      • main methods are not required

      removed

      • can make classes final

      can, but why bother?

      • That last CS has main() that accepts args which is different from other CSs. The main is really not required to understand what's going on. Also, println statements can be avoided because other CSs do not have this functionality.

      Yes it is, it illustrates how to catch suppressed exceptions.

      • This includes both any error while doing operations

        – what do you mean by error? Or do you mean subtype of IoException? s/error/exception?

      s/error/exception/g in that paragraph

      • When both the try block and also closing the reader throw an IOException, the catch clause catches both exceptions, and prints the try-block error as the thrown exception.

        – isn't this implicit? ...
      • Specify that you can also declare multiple resources in the try etc.

      I'm not convinced that either suggestion will make the rule better. We could argue all day about how much or little info to include without reaching a conclusion.

      • I also think it would be better to clarify that any exception in try that has the same type as any exception in the finally block will result in "exception swallowing". Such as read and write methods for IO also throw IOException and so does the IO stream classes (Bloch puzzle 41).

      The rule already does that, although s/swallow/ignore/g;

      • Bloch also suggests using the Closeable interface (instead of the dedicated method that we have listed as 2nd CS):

      Modified 2nd CS as suggested

      • This rule is not about abrupt termination of finally block but a reference to that rule will be useful.

      Added.

  7. If an operation fails, the failure should generally be reported by throwing an exception. All this "// Forward to handler" stuff is not the right way to handle the situation (except, perhaps, for failing upcalls).

    1. Well, first of all, we recommend that you not catch exceptions you are unprepared to handle. So when you see 'forward to handler', it's inside a catch clause, which means that we do not wish to (or cannot) throw the exception further, for various reasons.

      Second, the 'forward to handler' comments are actually referring to the Exception Reporter pattern promoted by ERR00-J. Do not suppress or ignore checked exceptions. I suppose that could be made more clear; if we want to standardize the wording we should agree on something and have it applied globally to every catch clause that has 'nonstandard' wording now.

  8. I changed "because IOExceptions are caught" to "because any IOException would be caught" in the first paragraph after the first noncompliant example because the s after IOException was causing the code close markup - }} - to fail.

  9. I can't understand the meaning of the phrase "The compiler correctly fails to diagnose this problem because ..." in the paragraph after the NCCE.
    can it read as "The compiler incorrectly fails to diagnose..." or, simply, "The compiler fails to diagnose..." ?

    another point I want to clarify is "the problem".
    does it mention about the compiler's failing to alert that the inner try block does not catch the exception?

    With my current understanding, the phrase should be re-written as "The compiler fails to diagnose that the inner try block does not catch the IOException, because any IOException would be caught by the outer catch block."

    1. Tweaked the wording; hopefully clearer now.

  10. Curious as to why this section specifically calls out "checked" exceptions?  This rule applies to unchecked exceptions as well.  While i understand that most RuntimeExceptions in the jdk are often "programmer error" type exceptions, you have frameworks like Hibernate where the primary exception (HibernateException) is a RuntimeException.  This rule should apply to all exceptions.

    Also, i agree with David Svoboda's earlier comment that this rule is pretty much just another case of ERR04-J.

    1. I can't see any reason for distinction between checked and unchecked and +1 on merging with ERR04-J. That said, in an earlier comment Dhruv says:

      This rule is not about abrupt termination of finally block ...

      but doesn't expand on this point.  In general, I think that both this rule and ERR04-J do not sufficiently motivate themselves to warrant inclusion in the standard. That's not to say there isn't good motivation, only that it doesn't appear in the text.