Why not just use java 7? What's stopping us? --Alex
> -----Original Message----- > From: Chiradeep Vittal [mailto:chiradeep.vit...@citrix.com] > Sent: Wednesday, December 11, 2013 1:28 PM > To: dev@cloudstack.apache.org > Subject: Re: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource > leaks > > +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
