Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Possible resource leak when reusing PreparedStatement?

Suppose you have the following code:

    Connection connection = null;
    PreparedStatement ps = null;

    try {
        Connection = connectionFactory.getConnection();

        ps = statement.prepareStamement(someQuery);
        // execute and read and stuff

        // now you want to use the ps again, since you don't want ps1, ps2, ps3, etc.
        ps = statement.prepareStatement(someOtherQuery); // DOES THIS FORM A POTENTIAL LEAK?
    } catch (a lot of exceptions) {
        // process exceptions
    } finally {

        // close the resources (using util class with null-checks and everything)
        SomeUtilClass.close(ps);
        SomeUtilClass.close(connection);
    }

Is reusing the ps variable a potential leak?

And if so, I would hate to declare multiple of such prepared statements (ps1, ps2, ps3, etc). How should I refactor this?

Thoughts anyone?

EDIT

Receiving several answers stating that this doesn't matter. I would like to point out that I'm experiencing open cursors that stay open a little bit too long, and was wondering if this pattern might have something to do with it.

My thoughts were:

The first statement gets dereferenced and GC'ed, so how does the first PreparedStatement get closed (Database-wise) in this example?

like image 635
bknopper Avatar asked Feb 28 '14 13:02

bknopper


2 Answers

This would not necessarily create a leak in a classic, "permanent", sense. The garbage collector will get to the first instance of the prepared statement eventually, and call its finalizer.

However, this is not a good practice to let the garbage collector deal with freeing potentially critical resources, such as DB handles: you should call the close method before reusing the prepared statement variable, or not reuse the variable at all.

try {
    Connection = connectionFactory.getConnection();

    ps = statement.prepareStamement(someQuery);
    // execute and read and stuff

    // now you want to use the ps again, since you don't want ps1, ps2, ps3, etc.
    // v v v v v v v v v v v
    SomeUtilClass.close(ps);
    // ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^
    ps = statement.prepareStatement(someOtherQuery); // DOES THIS FORM A POTENTIAL LEAK?
} catch (a lot of exceptions) {
    // process exceptions
} finally {

    // close the resources (using util class with null-checks and everything)
    SomeUtilClass.close(ps);
    SomeUtilClass.close(connection);
}
like image 117
Sergey Kalinichenko Avatar answered Oct 28 '22 03:10

Sergey Kalinichenko


In general yes, this could form a memory leak (of unmanaged resources). Without wishing to state the obvious, if a resource should be closed, then you should close it. In your example you are losing the reference to the first prepared statements and, as such, cannot close them.

It is likely that the GC and finaliser for the leaked reference will perform the neccessary steps to recalim memory from any unmanaged resources acquired, however, best practice dictates that you should be the one to deterministically perform this step.

In your example, in order to close all prepared statements, you could use an Iterable collection like so:

Deque<PreparedStatement> statements = new ArrayDeque<PreparedStatement>();
try {
  statements.addFirst(statement.prepareStamement(someQuery));
  PreparedStatement statement = statements.getFirst();
  ..
}...
finally {
  // enumerate statements and close them.
}
like image 26
Rich O'Kelly Avatar answered Oct 28 '22 03:10

Rich O'Kelly