cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Huang <Alex.Hu...@citrix.com>
Subject RE: [DISCUSS][PROPOSAL]Closing SQLStatement to avoid resource leaks
Date Wed, 11 Dec 2013 21:45:03 GMT
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"
> <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