2 posts
Home > Mikhail Ksenzov

 Mikhail Ksenzov

Klocwork Development Lead


Squeezing max from the ‘try/finally’ blocks

Posted by Mikhail Ksenzov   August 23rd, 2011

I often hear that closing resources properly is way too verbose in Java, especially considering that resource freeing methods such as ‘close()’ are often throwing some type of an exception. However, if you handle resources properly it might turn out to be less of a burden than one might think. Let’s start with the following snippet, where I use an SQL driver to retrieve the list of “codes” matching the given “id”:

09 List<String> requestCodes(String dbUrl, String id) {
10   List<String> result = new ArrayList<String>();
11   try {
12     Connection conn = DriverManager.getConnection(dbUrl);
13     PreparedStatement stmt = conn.prepareStatement("SELECT * FROM customers WHERE id = ?");
14     stmt.setString(1, id);
15     ResultSet rs = stmt.executeQuery();
16     while (rs.next()) {
17       result.add(rs.getString("code"));
18     }
19   } catch (SQLException e) {
20      e.printStackTrace();
21   }
22   return result;
23 }

The problem with the code above is that it allocates SQL server resources but fails to properly release them. More specifically:

  • Line 12: SQL connection ‘conn’ is not closed on exit.
  • Line 13: SQL object ‘stmt’ is not closed on exit.
  • Line 16: SQL object ‘rs’ is not closed on exit.

The next snippet illustrates how one can fix the defects listed above:

09 List<String> requestCodes(String dbUrl, String id) {
10   List<String> result = new ArrayList<String>();
11   Connection conn = null;
12   PreparedStatement stmt = null;
13   ResultSet rs = null;
14   try {
15     conn = DriverManager.getConnection(dbUrl);
16     stmt = conn.prepareStatement("SELECT * FROM customers WHERE id = ?");
17     stmt.setString(1, id);
18     rs = stmt.executeQuery();
19     while (rs.next()) {
20       result.add(rs.getString("code"));
21     }
22   } catch (SQLException e) {
23     e.printStackTrace();
24   } finally {
25     if (rs != null) {
26       try {
27         rs.close();          // close() throws an exception...
28       } catch (SQLException e) {
29         e.printStackTrace(); // ...have to catch it to free 'stmt'
30       }
31     }
32     if (stmt != null) {
33       try {
34         stmt.close();        // again, close() throws an exception...
35       } catch (SQLException ignore) {
36         e.printStackTrace(); // ...have to catch it to free 'conn'
37       }
38     }
39     if (conn != null) {
30       try {
41         conn.close();
42       } catch (SQLException ignore) {
43         e.printStackTrace();
44       }
45     }
46   }
47   return result;
48 }

The code above is correct but extremely verbose. However, it can be improved without sacrifices in semantics…

Tip #1: It is better to allocate a resource before the ‘try/finally’ block, not inside it. Let’s start with the following code:

Connection conn = null;
try {
  conn = DriverManager.getConnection(dbUrl);
  // use conn
} finally {
  if (conn != null) {
    conn.close();
  }
}

can be rewritten as:

Connection conn = DriverManager.getConnection(dbUrl);
try {
  // use conn
} finally {
  conn.close();
}

Tip #2: Use nested ‘try/finally’ blocks if you allocate a sequence of resources. Let’s start with a snippet:

try {
  Connection conn = DriverManager.getConnection(dbUrl);
  PreparedStatement stmt = conn.prepareStatement("SELECT * FROM customers WHERE id = ?");
  try {
    // use conn
    // use stmt
  } finally {
    try {
      conn.close();
    } catch (SQLException e) {
      e.printStackTrace();
    }
    try {
       stmt.close();
    } catch (SQLException e) {
      e.printStackTrace();
    }
  }
} catch (SQLException e) {
  e.printStackTrace();
}

How many problems have you noticed in the snippet above? I found three:

  • Allocation of resource ‘stmt’ can throw an exception before we enter the outer ‘try/catch/finally’. If happens ‘conn’ will never be freed.
  • We duplicate code for the SQLException handling. We were lucky to have only one line of code replicated, but it in general cases exception handling can be a bit more involved that we see here…
  • The order of resource allocation does not match the order of deallocation: here the order of deallocation should be reversed to be correct.

The only robust way to handle resource allocation/deallocation and to address the issues listed above is to use nested try/finally blocks:

try {
  Connection conn = DriverManager.getConnection(dbUrl);
  try {
    // use conn
    PreparedStatement stmt = conn.prepareStatement("SELECT * FROM customers WHERE id = ?");
    try {
      // use stmt
    } finally {
      stmt.close();
    }
  } finally {
    conn.close();
  }
} catch (SQLException e) {
  e.printStackTrace();
}

Let’s apply tips #1 and #2 to our original method and fix the resource leaks on lines 12, 13, 16:

09 List<String> requestCodes(String dbUrl, String id) {
10   List<String> result = new ArrayList<String>();
11   try {
12     Connection conn = DriverManager.getConnection(dbUrl);
13     try {
14       PreparedStatement stmt = conn.prepareStatement("SELECT * FROM customers WHERE id = ?");
15       try {
16         stmt.setString(1, id);
17         ResultSet rs = stmt.executeQuery();
18         try {
19           while (rs.next()) {
20             result.add(rs.getString("code"));
21           }
22         } finally {
23           rs.close();
24         }
25       } finally {
26          stmt.close();
27       }
28     } finally {
29       conn.close();
30     }
31   } catch (SQLException e) {
32     e.printStackTrace();
33   }
34   return result;
35 }

This is way shorter than the original solution!

Tip #3: If after applying tip #2 you feel that all your code drifted way too close to the right page margin it means that you probably have too much nested ‘try/finally’ blocks and that is time to check if you actually want to have all the resources allocated at the same time. Chances are that you do not need them all; otherwise use the Extract Method refactoring pattern to move out some of the resource access logic.

Tip #4: Know specific behavior of resources you are dealing with. While tips #1 – #3 provide a robust and compact approach for dealing with resource allocation/deallocation in general, in certain cases you can make code even more compact. In our example: Statement.close() closes its current ResultSet object if one exists. Likewise Connections.close() releases JDBC resources. It means that if you deal specifically with JDBC it would be sufficient to close the ‘parent’ resource to be sure that all ‘subresources’ will be properly released:

09 List<String> requestCodes(String dbUrl, String id) {
10   List<String> result = new ArrayList<String>();
11   try {
12     Connection conn = DriverManager.getConnection(dbUrl);
13     try {
14       PreparedStatement stmt = conn.prepareStatement("SELECT * FROM customers WHERE id = ?");
15       stmt.setString(1, id);
16       ResultSet rs = stmt.executeQuery();
17       while (rs.next()) {
18         result.add(rs.getString("code"));
19       }
20     } finally {
21       conn.close();
22     }
23   } catch (SQLException e) {
24     e.printStackTrace();
25   }
26   return result;
27 }

JSR 305: a silver bullet or not a bullet at all?

Posted by Mikhail Ksenzov   March 30th, 2009

JSR-305 is a Java Specification Request intended to improve the effectiveness of static analysis tools operating in Java 5+ environments. The idea here is that one can use special purpose annotations in order to provide static analysis tools with hints regarding the behaviour and side effects of methods.

An example of such annotations can be found in the presentation ‘Annotations for Software Defect Detection’ by William Pugh, who is masterminding the whole spec. Here we go:

 1: void test() {
 2:    if (spec != null) fFragments.add(spec);
 3:    if (isComplete(spec)) fPreferences.add(spec);
 4: }
 6:
 5: boolean isComplete(AnnotationPreferences spec) {
 6:    return spec.getColorPreferenceKey() != null
 7:        &amp;&amp; spec.getColorPreferenceValue() != null
 8:        &amp;&amp; spec.getTextPreferenceKey() != null
 9:        &amp;&amp; spec.getOverviewRulerPreferenceKey() != null;
10: }

What’s wrong with the snippet above? Well, the check for null on line 2 shows that the developer expects that the value of ‘spec’ can potentially be null, but it is still passed to method ‘isComplete’. Later, ‘isComplete’ attempts to dereference the value, which causes a NullPointerException.

According to Dr. Pugh, the best way to detect this issue statically is to force a developer to add the annotation @Nonnull to the method signature like this:

5: boolean isComplete(@Nonnull AnnotationPreferences spec) {

In this way, a basic static analysis tool that can minimally track ‘spec’ as a potential suspect for ‘null’ can issue a warning when the @Nonnull annotation is contradicted (that is, when ‘spec’ is passed to ‘isComplete’ as a parameter).

There are two problems with this approach:

  • it forces the developer to do work that rightly should be performed by a static analysis engine
  • it takes time to write the annotation for static analysis, but it takes even more effort to maintain the annotations and the actual code base in a consistent state.

In reality, the proposals behind JSR-305 exist to enable a tool intended for single function analysis (so-called intra-procedural analysis) to act as if it were performing whole program analysis by requiring the developer to state expected behaviour up front (whether or not that behaviour is actually expressed correctly in the developer’s code).

In contrast, this same scenario is supported by a whole program static analysis tool (so-called inter-procedural analysis) without developer intervention:

  1. First, a complete call-graph of the system is built, and then all the methods are ordered so that called methods are processed prior to callers — such an ordering allows the tool to generate all the necessary information about, in this instance, the method ‘isComplete’ by the time the analysis of the method ‘test’ begins.
  2. During the analysis of ‘isComplete’, the tool records the fact that the incoming argument ‘spec’ is dereferenced.
  3. Next, the method ‘test’ is analyzed. In this method, the variable ‘spec’ is checked for null, so it is tracked as a potential suspect for an exception. Using the information generated about ‘isComplete’ the tool can reliably issue a warning on line 3, since it already knows that ‘isComplete’ dereferences the incoming argument.

So that example applies to a simple unconditional dereference scenario. In more complicated cases, Dr. Pugh proposes to use the annotation parameter ‘when’, with one of the following values:

  • ALWAYS
  • NEVER
  • MAYBE
  • UNKNOWN

For example: ‘@Nonnull(when=When.NEVER)’ means that a value is always null in the given context.

This specification seems to be a compromise between the amount of information provided by a developer to a static analysis tool and the amount of effort a developer has to put into it, a compromise that does not seem to be a particularly good solution here. First of all, the amount of information provided in such a manner is insufficient to provide accurate analysis, and secondly this seems to be too much work for the developer, especially when this work can be avoided.

Let’s examine how conditional value dereferencing is supported by whole program static analysis tools:

 1: void test() {
 2:     entity.qualifiedName = null;
 3:     saveName(entity, false);
 4: }
 5:
 6: boolean    saveName(Entity entity, boolean qualified) {
 7:    String name;
 8:    if (qualified)
 9:        name = entity.qualifiedName.trim();
10:    else
11:        name = entity.name.trim()
12:
13:     save(name);
14: }

In this example, an inter-procedural static analysis tool would first analyze the method ‘saveName’. A good analysis engine should be able to record the fact that this method only dereferences ‘entity.qualifiedName’ if the second parameter, ‘qualified’, is set to ‘true’. This, it would appear, is a deal more detailed than one can practically achieve by adding @Nonnull(when=When.XXX) annotations, even with all the work the annotation implies for the developer.

Next, the method ‘test’ would be analyzed. A good static analysis tool will naturally keep track of ‘entity.qualifiedName’ because of the assignment to ‘null’ on line 2 and its therefore potential use in an exception causing context. However, given that the actual call to ‘saveName’ on line 3 uses ‘false’ as its second argument, such a tool will not issue a warning that would in reality be a false positive, since the knowledge gained from analyzing ‘saveName’ disqualifies any potential warning due to the conditional relationship between arguments.

In summary, JSR-305 proposes a whole roster of interesting ideas for using annotations to enhance static analysis of Java code, and NPE detection seems to be only one aspect of this specification request. In upcoming blog posts, we shall continue the discussion of proposed annotations as well as offering our own ideas about how and when annotations should be used in static analysis.