Patbox / BanHammer

Simple punishment utility for Fabric
GNU Lesser General Public License v3.0
12 stars 3 forks source link

Introduce SQL Connection Pooling #30

Closed ChristopherHaws closed 1 year ago

ChristopherHaws commented 1 year ago

Hey there,

Thanks for this great mod! I have been running into connection issues when using the MySQL database provider (I am going to be switching to the new PostgreSQL provider as soon as I update the mod but it looks like it also doesn't use connection pooling). After my server has been running for a long time (few days), banhammer stops being able to communicate with the database and I have to reboot my server.

After looking through the code, it looks like you are initializing the database connection 1 time at mod initialization and not closing the connection until the server shuts down. It also looks like you aren't closing the PreparedStatement or ResultSet after using it which is a memory leak and can cause connection issues. The issue with keeping the connection open all the time is that the SQL server will eventually cleanup the connection after a certain amount of time making the connection invalid. Also, a single connection is not thread safe so it should always be called in a single thread.

Generally, the way you are supposed to manage database connections is by pooling the connections and letting the pool manage the state of the connection (it will check if the connection is valid and if it isn't it will attempt to create a new connection). The code that needs to call the SQL server should call getConnection from the pool in a try/catch and should always close the connection in a finally statement.

Here is an example of how to make a sql call using a connection pool:

Connection connection = null;
PreparedStatement statement = null;

try {
    try {
        connection = connectionPool.getConnection();
        statement = connection.prepareStatement("...");
        // Setup statement parameters
        // Execute statement
    } finally {
        if (statement != null) {
            statement.close();
        }

        if (connection != null) {
            connection.close();
        }
    }
} catch (SQLException ex) {
    ex.printStackTrace();
}

or after Java 7, using this less verbose syntax:

try {
    try (
        var connection = connectionPool.getConnection();
        var statement = connection.prepareStatement("...");
    ) {
        // Setup statement parameters
        // Execute statement
    }
} catch (SQLException ex) {
    ex.printStackTrace();
}

Here are some links where you can read more about this:

I am not sure if you can depend on one of the many libs for pooling the connection in Minecraft, but if not, here is a simple, 1 class, implementation of one that can be embedded directly into your mod:

Patbox commented 1 year ago

Thanks for all info! Outside of BanHammer I didn't really work much with databases, so I am rather inexperienced there.

ChristopherHaws commented 1 year ago

Additionally, if you could set some custom properties on the connection, it would help debugging issues in the sql server (it makes it so we can tell which mod owns that connection).

Some properties that would help are:

ChristopherHaws commented 1 year ago

Thanks for all info! Outside of BanHammer I didn't really work much with databases, so I am rather inexperienced there.

No worries! If you have any question or would like some help, feel free to ping me. This is what open source is all about ^_^

Patbox commented 1 year ago

Implemented the basics and from my quick testing it still works™ Haven't added debugging stuff yet (tbf forgot about it and now it's released). Anyway, thanks for help!