Using HikariCP's connection pool the correct way

Question

I been trying to develop a Minecraft server plugin where a player enters a command with some data, data is sent to database, or, a command that requests some data from database.

It's working, until a user starts using it more then a few times. I get a leakdetection error:

[HikariPool-2 housekeeper] WARN com.zaxxer.hikari.pool.ProxyLeakTask - Connection leak detection triggered for com.mysql.jdbc.JDBC4Connection@abc6eb, stack trace follows
[23:36:11 WARN]: java.lang.Exception: Apparent connection leak detected

Or I get an error that tells me that I have too many connections. (Sorry, I don't have that error at this moment)

This is the gist of my code. What am I doing improperly?

public class MochaModel {

    private Latte instance = Latte.getInstance();
    private Connection connection;


    public MochaModel() {

    }

    public void createTable() {
        BukkitRunnable r = new BukkitRunnable() {
            @Override
            public void run() {
                try {
                    connection = Database.getConnection();
                    if (connection != null) {
                        String sql = "CREATE TABLE IF NOT EXISTS `mocha` ( " +
                                " `id` INT NOT NULL AUTO_INCREMENT ," +
                                "`uuid` VARCHAR(255) NOT NULL ," +
                                " `join_message` VARCHAR(255) NOT NULL ," +
                                " `quit_message` VARCHAR(255) NOT NULL ," +
                                " `change_points` INT NOT NULL," +
                                " `last_modified` TIMESTAMP NOT NULL," +
                                " PRIMARY KEY (`id`)" +
                                ")";
                        PreparedStatement q = connection.prepareStatement(sql);
                        q.executeUpdate();
                    }
                } catch(SQLException e) {
                    e.printStackTrace();
                } finally {
                    try {
                        if (connection != null) {
                            connection.close();
                        }
                    } catch (SQLException e) {
                        e.printStackTrace();
                    }
                }
            }
        };

        r.runTaskAsynchronously(instance);
    }

    public void setJoinMessage(String uuid, String message) {
        ResultSet rs = getDataWithUUID(uuid);
        String[] sqlValues = new String[2];
        try {
            if (!rs.isBeforeFirst()) {
                String insertSql = "INSERT INTO `mocha` (`uuid`, `join_message`,`quit_message`, `change_points`, `last_modified`) VALUES (?, ?, '', 0, CURRENT_TIMESTAMP)";
                sqlValues[0] = uuid;
                sqlValues[1] = message;
                insertData(insertSql, sqlValues);
            } else {
                while (rs.next()) {
                    String updateSql = "UPDATE `mocha` SET `join_message`=? WHERE `uuid`=?";
                    sqlValues[0] = message;
                    sqlValues[1] = uuid;
                    updateData(updateSql, sqlValues);
                }
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }

    public void setQuitMessage(String uuid, String message) {
        ResultSet rs = getDataWithUUID(uuid);
        String[] sqlValues = new String[2];
        try {
            if (!rs.isBeforeFirst()) {
                String insertSql = "INSERT INTO `mocha` (`uuid`, `join_message`,`quit_message`, `change_points`, `last_modified`) VALUES (?, '', ?, 0, CURRENT_TIMESTAMP)";
                sqlValues[0] = uuid;
                sqlValues[1] = message;
                insertData(insertSql, sqlValues);
            } else {
                while (rs.next()) {
                    String updateSql = "UPDATE `mocha` SET `quit_message`=? WHERE `uuid`=?";
                    sqlValues[0] = message;
                    sqlValues[1] = uuid;
                    updateData(updateSql, sqlValues);
                }
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }

    private void updateData(String sql, String[] sqlValues) {
        BukkitRunnable r = new BukkitRunnable() {
            @Override
            public void run() {
                try {
                    connection = Database.getConnection();
                    if (connection != null) {
                        PreparedStatement q = connection.prepareStatement(sql);
                        q.setString(1, sqlValues[0]);
                        q.setString(2, sqlValues[1]);
                        System.out.println(q);
                        q.executeUpdate();
                    }
                } catch(SQLException e) {
                    e.printStackTrace();
                } finally {
                    try {
                        if (connection != null) {
                            connection.close();
                        }
                    } catch (SQLException e) {
                        e.printStackTrace();
                    }
                }
            }
        };
        r.runTaskAsynchronously(instance);
    }

    private void updateChangePointsData(String sql, String[] sqlValues) {
        BukkitRunnable r = new BukkitRunnable() {
            @Override
            public void run() {
                try {
                    connection = Database.getConnection();
                    if (connection != null) {
                        PreparedStatement q = connection.prepareStatement(sql);
                        q.setInt(1, Integer.parseInt(sqlValues[0]));
                        q.setString(2, sqlValues[1]);
                        System.out.println(q);
                        q.executeUpdate();
                    }
                } catch(SQLException e) {
                    e.printStackTrace();
                } finally {
                    try {
                        if (connection != null) {
                            connection.close();
                        }
                    } catch (SQLException e) {
                        e.printStackTrace();
                    }
                }
            }
        };
        r.runTaskAsynchronously(instance);
    }

    private void insertData(String sql, String[] sqlValues) {
        BukkitRunnable r = new BukkitRunnable() {
            @Override
            public void run() {
                try {
                    connection = Database.getConnection();
                    if (connection != null) {
                        PreparedStatement q = connection.prepareStatement(sql);
                        q.setString(1, sqlValues[0]);
                        q.setString(2, sqlValues[1]);
                        System.out.println(q);
                        q.executeUpdate();
                    }
                } catch(SQLException e) {
                    e.printStackTrace();
                } finally {
                    try {
                        if (connection != null) {
                            connection.close();
                        }
                    } catch (SQLException e) {
                        e.printStackTrace();
                    }
                }
            }
        };
        r.runTaskAsynchronously(instance);
    }

    private ResultSet getDataWithUUID(String uuid) {
        ResultSet result = null;
        String sqlPlayer = "SELECT * FROM `mocha` WHERE `uuid` = ?";
        try {
            connection = Database.getConnection();
            if (connection != null) {
                PreparedStatement q = connection.prepareStatement(sqlPlayer);
                q.setString(1, uuid);
                result = q.executeQuery();
            }
        } catch(SQLException e) {
            e.printStackTrace();
        }

        return result;
    }

    public String getMessage(String uuid, String messageType) {
        ResultSet rs = getDataWithUUID(uuid);
        String message = null;
        try {
            if (!rs.isBeforeFirst()) {
                message = null;
            } else {
                while (rs.next()) {
                    if (messageType.equalsIgnoreCase("getjoin")) {
                        message = rs.getString("join_message");
                    } else if (messageType.equalsIgnoreCase("getquit")) {
                        message = rs.getString("quit_message");
                    }
                }
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return message;
    }

    public int getChangePoints(String uuid) {
        ResultSet rs = getDataWithUUID(uuid);
        int changePoints = 0;
        try {
            if (!rs.isBeforeFirst()) {
                changePoints = 0;
            } else {
                while (rs.next()) {
                    changePoints = rs.getInt("change_points");
                }
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return changePoints;
    }

    public void removeChangePoints(String uuid, int amount) {
        int changePoints = getChangePoints(uuid);
        String[] sqlValues = new String[2];
        if (changePoints >= amount) {
            String updateSql = "UPDATE `mocha` SET `change_points`=? WHERE `uuid`=?";
            sqlValues[0] = String.valueOf((changePoints-amount));
            sqlValues[1] = uuid;
            updateData(updateSql, sqlValues);
        }
    }
    public void addChangePoints(String uuid, int amount) {
        int changePoints = getChangePoints(uuid);
            String[] sqlValues = new String[2];
            String updateSql = "UPDATE `mocha` SET `change_points`=? WHERE `uuid`=?";
            sqlValues[0] = String.valueOf((changePoints+amount));
            sqlValues[1] = uuid;
            updateChangePointsData(updateSql, sqlValues);
    }
}

My DB Class:

public class Database {
    private static Latte instance = Latte.getInstance();
    private static Config config = new Config();
    private static HikariConfig dbConfig;

    static {

        dbConfig = new HikariConfig();
        dbConfig.setJdbcUrl("jdbc:mysql://localhost:3306/" + config.get("database.database"));
        dbConfig.setUsername(config.get("database.username"));
        dbConfig.setPassword(config.get("database.password"));
        dbConfig.setDriverClassName("com.mysql.jdbc.Driver");
        dbConfig.addDataSourceProperty("cachePrepStmts", "true");
        dbConfig.addDataSourceProperty("prepStmtCacheSize", "250");
        dbConfig.addDataSourceProperty("prepStmtCacheSqlLimit", "2048");
    }

    private static HikariDataSource ds = new HikariDataSource(dbConfig);

    public static Connection getConnection()  {
        try {
            ds.setIdleTimeout(60000);
            ds.setConnectionTimeout(60000);
            ds.setValidationTimeout(3000);
            ds.setLoginTimeout(5);
            ds.setMaxLifetime(60000);
            ds.setMaximumPoolSize(20);
            ds.setLeakDetectionThreshold(5000);
            return ds.getConnection();
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return null;
    }

}

Corking

You store Connection as a class level variable, you shouldn't do this. Due to this, you might open 3 connections but only close the last one. Each time you open a new Connection you are overriding the previous one and thus leaking it. – Proboscis

Thanks, how would I properly do this? That totally makes sense. – Corking

Answer 1

When opening a Connection you also need to close it. However you are storing the Connection in a instance variable. Which, for certain paths in your code, might result in multiple Connection instances being used. Due the the storage in the instance variable only the last one used will get closed, all the others are leaked.

Instead you want to make it local or hide parts of the complexity. You could rewrite your Database class to something like this.

Note: Assuming Java 8 here!

public class Database {
    private static Latte instance = Latte.getInstance();
    private static Config config = new Config();
    private static HikariConfig dbConfig;

    static {

        dbConfig = new HikariConfig();
        dbConfig.setJdbcUrl("jdbc:mysql://localhost:3306/" + config.get("database.database"));
        dbConfig.setUsername(config.get("database.username"));
        dbConfig.setPassword(config.get("database.password"));
        dbConfig.setDriverClassName("com.mysql.jdbc.Driver");
        dbConfig.addDataSourceProperty("cachePrepStmts", "true");
        dbConfig.addDataSourceProperty("prepStmtCacheSize", "250");
        dbConfig.addDataSourceProperty("prepStmtCacheSqlLimit", "2048");
    }

    private static HikariDataSource ds = new HikariDataSource(dbConfig);

    public static <T> T execute(ConnectionCallback<T> callback) {
      try (Connection conn = ds.getConnection()) {
        return callback.doInConnection(conn);
      } catch (SQLException e) {
           throw new IllegalStateException("Error during execution.", e);
      }
    }

    public static interface ConnectionCallback<T> {
        public T doInConnection(Connection conn) throws SQLException;
    }
}

Notice no more getConnection and due to the try-with-resources the connection will get closed automatically.

You can now call this method with instances of ConnectionCallback instead of getting the Connection and manage it yourself.

Now the code that uses the Connection can be refactored, to something like this. (Notice no more catches, closes etc. all that is handled in the Database.execute method.

private void updateData(String sql, String[] sqlValues) {
    BukkitRunnable r = new BukkitRunnable() {
        @Override
        public void run() {
            Database.execute( (conn) -> {
                PreparedStatement q = conn.prepareStatement(sql);
                q.setString(1, sqlValues[0]);
                q.setString(2, sqlValues[1]);
                System.out.println(q);
                q.executeUpdate();
                return null;
            }} );
    };
    r.runTaskAsynchronously(instance);
}

This code will close the Connection after each use (and you cannot forget to close it).

Proboscis

Thank you SO much for the explanation! I am trying to get your code working in my code, however I am having an issue with the closing braces. gist.github.com/anonymous/a4900271e808a93bb60cce83e68f63ee In the closing three brackets/braces above r.runTask... I'm getting a few errors, depending on which one I hover for the error with. Missing return statement, expected ) and ; and expected ;.. Really weird errors and I've tried everything. – Corking

Also in the execute method, I'm getting an error that says it's missing a return statement. – Corking

Well I typed it in the answer, so could be that the aligning is off :). There is a issue in the execute method. The return value should be first assigned to a value and returned at the end of the method or the exception rethrown. In the other code there should be a ; just as I did in the sample. – Proboscis

Just add return null; inside the block. – Proboscis

I'm sorry but that continues to error out. Which line in what block? – Corking

after the q.executeUpdate(). – Proboscis

is this code even doing connection pooling, why use hikari at all? – Aerolite

Yes it is, because a pool is being used stored in a class variable. Default pool size in Hikari is 10. – Proboscis