Java SQL memory leak

Question

I'm facing an issue where I have a java application running on a server, and it starts growing in memory until eventually the server cannot handle it anymore.

This is some sort of memory leak / resource leak problem, which I thought was extremely rare in Java due to the garbage collection. I guess something is being referenced and never used, so the garbage collector does not collect it.

The problem is that the size in memory grows so slowly that I'm not able to debug it properly (it may take two weeks to make the server unusable). I'm using java + mysql-connector, and I'm sure the memory leak is caused by something related to the database connection.

Here is how I connect to the database:

 private static Connection connect(){
    try {
        Connection conn = null;
        conn =  DriverManager.getConnection("jdbc:mysql://localhost:3306/database","client","password");
        return conn;
    }catch(SQLException ex){
        System.out.println("SQLException: " + ex.getMessage());
        System.out.println("SQLState: " + ex.getSQLState());
        System.out.println("VendorError: " + ex.getErrorCode());
        return null;
    }
}
public static Connection getConnection(){
    try {
        if (connection == null || connection.isClosed()) connection = connect();
        return connection;

    }catch (SQLException exception){
        System.out.println("exception trying to connect to the database");
        return null;
    }
}

I can't find any possible problem here, but who knows!

Here's how I retrieve information from the database:

 public void addPoints(long userId,int cantidad){
    try {
        if(DatabaseConnector.getConnection()!=null) {
            PreparedStatement stm = DatabaseConnector.getConnection().prepareStatement("UPDATE users SET points = points + ? WHERE id = ? ");
            stm.setLong(2, userId);
            stm.setInt(1, cantidad);
            if(stm.executeUpdate()==0){ //user doesn't have any point records in the database yet
                PreparedStatement stm2 = DatabaseConnector.getConnection().prepareStatement("INSERT INTO users (id,points) VALUES (?,?)");
                stm2.setLong(1, userId);
                stm2.setInt(2, cantidad);
                stm2.executeUpdate();
            }
        }
    }catch (SQLException exception){
        System.out.println("error recording points");
    }
}

 public ArrayList<CustomCommand> getCommands(long chatId) throws SQLException{
    ArrayList<CustomCommand> commands = new ArrayList<>();
    if(DatabaseConnector.getConnection() != null) {
        PreparedStatement stm = DatabaseConnector.getConnection().prepareStatement("SELECT text,fileID,commandText,type,probability FROM customcommands WHERE chatid = ?");
        stm.setLong(1, chatId);
        ResultSet results = stm.executeQuery();
        if(!results.isBeforeFirst()) return null;
        while (results.next()){
            commands.add(new CustomCommand(results.getString(1),results.getString(2),results.getString(3), CustomCommand.Type.valueOf(results.getString(4)),results.getInt(5)));
        }
        return commands;
    }
    return null;
}

Maybe the problem is something related to exception catching and statements not being correctly executed? Maybe something related to result sets? It's driving me crazy. Thanks for helping me!

Please have a look at how to close JDBC resources properly using [try with resources](https://stackoverflow.com/questions/8066501/how-should-i-use-try-with-resources-with-jdbc). You're also creating and discarding one `Connection` in the null-checks that you have in the beginning of each method and that's unnecessary. — Mick Mnemonic, Jan 24 '22 at 20:02

score 2 · Accepted Answer · answered Jan 24 '22 at 19:57

2

You do nothing to clean up ResultSet and Statement before you return. That's a bad idea. You should be closing each one in individual try/catch blocks in a finally block.

A ResultSet is an object that represents a database cursor on the database. You should close it so you don't run out of cursors.

I wouldn't have a single static Connection. I'd expect a thread-safe, managed pool of connections.

I wouldn't return a null. You don't make clear what the user is supposed to do with that. Better to throw an exception.

answered Jan 24 '22 at 19:57

duffymo

305,152
44
369
561

It's just a telegram bot, so I am handling the errors myself, it's not a big project, that's why I am not creating exceptions and only have one static connection. Thank you very much for your help, I've changed the code closing each statement / result set in finally blocks or after returning. If this works, I will return here and change the state to solved :) – tremon Jan 24 '22 at 21:15
No, you must close them in a finally block in the scope of the method in which they were created. Each one has to be wrapped in an individual try/catch block so both are done. They must be closed even if an exception is thrown. – duffymo Jan 24 '22 at 21:16

Java SQL memory leak

1 Answers1