views:

118

answers:

4

Hi!

I am experiencing a memory leak with a code similar to the one below (it's a simulation with different inputs at every loop).

The problem

The object Object_XXX is quite complex, with connections to databases and other objects populated with data from databases aswell.

    for(int i=0; i<MAX; i=i+1){
        Class_XXX Object_XXX = new Class_XXX(Arg_1, Arg_2);

        // action with Object_XXX
    }

Now, after calling a couple of methods, Object_XXX can also be discarded since the next loop will requires an object with different characteristics (different arrays, size of the arrays, nested objects...).

The constructor is similar to the one below, and the other classes have a similar constructor.

public Class_XXX(Arg_1, Arg_2, DB_Connection){

    try {
        Statement Query_Statement = null;
        ResultSet Query_ResultSet = null;
        String Query_String = null;

        Query_String = "...";
        Query_Statement = DB_Connection.createStatement();
        Query_ResultSet = Query_Statement.executeQuery(SQL);

        while (Query_ResultSet .next()) {
            this.Param_1 = Query_ResultSet .getString("param_1");
    this.Param_2 = Query_ResultSet .getString("param_2");
            ...
    this.Param_n = Query_ResultSet .getString("param_n");
        }
    } catch (SQLException e) {
        System.out.println("SQL Exception: "+ e.toString());
    }
}

Questions

What would be the most correct approach in this case? a) to finalize Object_XXX at the end of the loop b) to finalize every single object that compose Object_XXX when they are not used inside the code? Personally I would prefer a) since I think this would leave the garbage collector to work without messing too much with it

Could you also provide a code example or a reference?

Thanks!


Second round:

After the answers found below and a look at this other page (http://accu.org/index.php/journals/236), this is the template that i'm using now for the constructors. Too early to see if it works. There is still the "exception.toString" but the real code gives to the variables standard values in the case of an exception and reports the action in a log.

public Class_XXX(String Object_Name, java.sql.Connection Query_Connection){

    try{ // begin try-catch for Query_Connection
        Statement Query_Statement = Query_Connection.createStatement();
        try { // begin try-finally for Query_Statement
            String Query_String = "SELECT param_1, param_2, ... param_3 FROM table_name WHERE object_name = '" + Object_Name + "'";
            ResultSet Query_ResultSet = Query_Statement.executeQuery(Query_String);
            try { // begin try-finally for Query_ResultSet

                while (Query_ResultSet.next()) {
                    this.Param_1 = Query_ResultSet.getString("param_1");
                    this.Param_2 = Query_ResultSet.getString("param_2");
                    // ...
                    this.Param_n = Query_ResultSet.getString("param_n");
                }

            } finally {
                try { Query_ResultSet.close(); }
                catch (SQLException ex) { System.out.println("Error in Class_XXX constructor - " + ex.toString()); }
            } // end try-finally for Query_ResultSet

        } finally {
            try { Query_Statement.close(); }
            catch (SQLException ex) { System.out.println("Error in Class_XXX constructor - " + ex.toString()); }
        } // end try finally for Query_Statement

    } catch(SQLException ex) {
            System.out.println("Error in Class_XXX constructor - " + ex.toString());
    } // end try-catch for Query_Connection

}
+4  A: 

Finalizing object won't delete it from memory if something still holds reference to it. And if nothing holds the reference, garbage collector will delete it anyway, so finalizing doesn't make much sense here.

If you are experiencing memory leak, something must keep holding reference to your objects, thus they cannot be garbage collected. I'd suggest to use some profiler to see what is it.

amorfis
+1. Looking at the sample code, i'm pretty sure they're not closing most of their objects, such as ResultSets and thus have multiple leaks. Forcing GC or setting objects to null won't help here, there's too much other hidden stuff like JDBC and the strange DB_Connection class.
mhaller
Ja, the profiler shows that the problem is actually in the JDBC.
andystrath
+1  A: 

Using finalizing approach is not recommended. You would better leave this to garbage collector. BUT, you should release nested resources (close them, assign null).

Andriy Sholokh
assigning null to a variable that holds a limited resource will in most cases lead to a quick program degradation (because it may never be GCed). Basically, that's what's happening here, because once his constructor ends all local variables become available for GC.
Alexander Pogrebnyak
@Alexander Pogrebnyak You are absolutely correct. What I meant is objects that point some external resources should be released explicitly. The same is for array of objects. Assigning NULL is OK for standalone objects.
Andriy Sholokh
+1  A: 

Boy, what a constructor.

You've allocated quite a few local variables, and did not show us any code that releases them, e.g.

    Statement Query_Statement = null;
    ResultSet Query_ResultSet = null;

Although you should not call finalize yourself in any case, calling it here will not help you anyway, because it will not have access to the local variables declared in your constructor.

Learn to follow this pattern:

final Statement stmt = createStatement( );

try
{
  useStatement( stmt );
}
finally
{
  stmt.close( );
}

That's the only way to prevent resource ( not just memory ) leaks.

Also, it's a VERY bad idea to swallow exception the way you do it.

Alexander Pogrebnyak
+2  A: 

Looks like you are a C++ programmer used to doing RAII.

Java does not support RAII with equivalent semantics as C++, as object destruction is done by the garbage collector, sometime after the object becomes unreachable, in a seperate background thread. Because of this, hardly anyone uses the finalize method (which would otherwise be the equivalant of a C++ destructor) to release resources.

For objects that only occupy memory, Java does not need RAII, as their memory will automatically be reclaimed by the garbage collector sometime after they become unreachable. In particular, explicitly freeing members is not necessary.

Objects managing resources other than memory (such as file descriptors), or wishing to perform immediate cleanup work usually offer a cleanup-method, which must be invoked explicitly. As you can see from their javadoc, instances of Statement and ResultSet are such objects (they refer to resources outside the vm, which one wants to release in a timely fashion). The typical pattern to invoke the cleanup method in an exception-safe manner is:

Statement statement = connection.createStatement();
try {
    ResultSet resultset = statement.executeQuery(sql);
    try {
        // read the resultset
    } finally {
        resultset.close();
    }
} finally {
    statement.close();
}

And a few matters of style:

  • exception.toString() merely contains the exception message. exception.printStackTrace() additionally prints the entire stacktrace.
  • nearly every java programmer follows the convention that package names begin with a lowercase letter, class names with an upper case letter, and fields/variables with lowercase letters. Also, words are usually separated using camel-case rather than _.
meriton
Thanks! For the example and the explication. We'll see if it works when I'll implement it in my 1000s classes. Never heard of this CamelCase before, but a quick search on Wikipedia pointed me straight to this: http://en.wikipedia.org/wiki/CamelCase#The_.22Lazy_Programmer.22_theory I'll join the mainstream as soon as possible!
andystrath