+1
Wish there was a more elegant solution (like in Java 7)
On 12/10/13 6:01 AM, "Antonio Fornié Casarrubios"
<antonio.for...@gmail.com> wrote:
>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
