Hi all,
I'm trying to fix some typical "Resource Leak" issues in some cloudstack
classes. An example could be Upgrade2214to30, but actually the very same
issue can be found in many other classes.
The reason for this mail is all these occurrences shall be fixed in a
congruent way and I would like to know your thoughts about the following
proposal:
Let's see this exaple code
01 PreparedStatement pstmt = conn.prepareStatement("SELECT bla bla bla");
02 ResultSet rs = pstmt.executeQuery();
03 while (rs.next()) {
04 pstmt = conn.prepareStatement("INSERT INTO bla bla");
05 pstmt.setLong(1, networkOfferingId);
06 pstmt.executeUpdate();
07 }
In line 4 we assign a new PrepSt object to the same variable, so the
previous object is lost and impossible to .close() and the same will
happen in each following iteration of the loop. Of course we cannot close
the first PrepSt because that would also close the ResultSet and thus brake
the loop, so we could create a second variable insertPstmt and change it
like this:
01 PreparedStatement pstmt = conn.prepareStatement("SELECT bla bla bla");
02 ResultSet rs = pstmt.executeQuery();
03 while (rs.next()) {
04 PreparedStatement insertPstmt = conn.prepareStatement("INSERT
INTO bla bla");
05 insertPstmt.setLong(1, networkOfferingId);
06 insertPstmt.executeUpdate();
07 insertPstmt.close();
08 }
...
15 finally{
16 if (pstmt != null) {
17 pstmt.close();
18 }
19 }
This solution could be good for simple scenarios, but in some others we
have several PrepSt open at the same time with 2 or 3 levels of nested
loops... so this solution would mean creating plenty of PrepSt variables,
which I think could be messy and error-prone...
...and on top of that we end up repeating the same code in all the finally
blocks of all the methods as we do now:
} finally {
try {
if (rs != null) {
rs.close();
}
if (pstmt != null) {
pstmt.close();
}
} catch (SQLException e) {
}
}
I propose that in each of these cases we would do something like:
00 List<PreparedStatement> pstmt2Close = new
ArrayList<PreparedStatement>();
01 PreparedStatement pstmt = conn.prepareStatement("SELECT bla bla bla");
02 pstmt2Close.add(pstmt);
03 ResultSet rs = pstmt.executeQuery();
04 while (rs.next()) {
05 pstmt = conn.prepareStatement("INSERT INTO bla bla");
06 pstmt2Close.add(pstmt);
06 pstmt.setLong(1, networkOfferingId);
07 pstmt.executeUpdate();
08 }
...
15 finally{
16 closePstmts(pstmt2Close);
17 }
So we have a single List of Statements where I drop all of them as they are
created and in the finally block I call this method:
01 protected void closePstmts(List<PreparedStatement> pstmt2Close){
02 for(PreparedStatement pstmt : pstmt2Close) {
03 try {
04 if (pstmt != null && !pstmt.isClosed()) {
05 pstmt.close();
06 }
07 } catch (SQLException e) {
08 // It's not possible to recover from this and we need to
continue closing
09 e.printStackTrace();
10 }
11 }
12 }
Note1: Once a PreparedStatment is closed it's Resultset is closed too, so
this method doesn't need more.
Note2: We code the finally block only once and for all, instead of having
different developers repeating the same or even worse, coding something
different (Ana prints the Exception msg, Bob doesn't print and Carl throws
a new Ex)
Note3: For next versions of Java (from 1.7 on) PreparedStatement and other
classes implement AutoCloseable, so we can have a list of AutoCloseable and
if we want our method cleaner we can even use DbUtils.closeQuietly().
If you guys don't see any drawbacks I will proceed and do the same in
future similar scenarios.
Thanks. Cheers
antonio
