You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Antonio Fornié Casarrubios <an...@gmail.com> on 2013/12/10 15:01:07 UTC
[DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
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
RE: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
Posted by Alex Huang <Al...@citrix.com>.
Thanks Chip. I've forgotten about that. I don't think "potentially alienating someone" is never a good reason for us to do or not do something. If there was nothing to gain from updating jdk to 1.7, then we might as well just leave it at 1.6. If there are good reasons, then why not? I think there's good reasons proposed in this thread.
--Alex
> -----Original Message-----
> From: Chip Childers [mailto:chipchilders@apache.org]
> Sent: Wednesday, December 11, 2013 4:51 PM
> To: dev@cloudstack.apache.org
> Subject: Re: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource
> leaks
>
> On Wed, Dec 11, 2013 at 09:45:03PM +0000, Alex Huang wrote:
> > Why not just use java 7? What's stopping us?
> >
> > --Alex
>
> This is a relevant thread with stated objections:
>
> http://markmail.org/message/a4lfcbqwi7y474hy
Re: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
Posted by Chip Childers <ch...@apache.org>.
On Wed, Dec 11, 2013 at 09:45:03PM +0000, Alex Huang wrote:
> Why not just use java 7? What's stopping us?
>
> --Alex
This is a relevant thread with stated objections:
http://markmail.org/message/a4lfcbqwi7y474hy
RE: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
Posted by Alex Huang <Al...@citrix.com>.
Why not just use java 7? What's stopping us?
--Alex
> -----Original Message-----
> From: Chiradeep Vittal [mailto:Chiradeep.Vittal@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"
> <an...@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
Re: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
Posted by Chiradeep Vittal <Ch...@citrix.com>.
+1
Wish there was a more elegant solution (like in Java 7)
On 12/10/13 6:01 AM, "Antonio Fornié Casarrubios"
<an...@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