cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chiradeep Vittal <Chiradeep.Vit...@citrix.com>
Subject Re: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
Date Wed, 11 Dec 2013 21:27:38 GMT
+1
Wish there was a more elegant solution (like in Java 7)

On 12/10/13 6:01 AM, "Antonio ForniƩ Casarrubios"
<antonio.fornie@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


Mime
View raw message