Reading data while the database is not available is not handled correctly

[seyfahni]

Based on static code analysis of code based on BQ version 2.0.0-688 from #2469:

Imagine this: You need to load player data, but the database is offline.

With the following setup:

1. PlayerData.loadAllPlayerData() is called.
2. That method creates a local variable: Connector con = new Connector()
3. In the constructor of Connector the database connection fetched from the Database instance stored by the plugin: connection = database.getConnection();

There are two cases:

🅰️ The case that there is no open connection:

1. The connection is opened by the Database: con = openConnection()
2. The MySQL implementation fails to open a connection, logs "MySQL says: ..." (something about not being able to establish a connection) as a warning and returns null
3. No error reaches getConnection() of the Database class, it's con class field is set to null and it returns null
4. The Connector (still inside the constructor) sets the connection field to null that it received from the Databaseclass
5. PlayerData tries to query the player's data with con.querySQL(...)
6. That method executes PreparedStatement statement = connection.prepareStatement(sql) without doing any null or connection validity checks, causing a NullPointerException

🅱️ The case that there already is an open (but dead) connection:

1. The dead connection still stored in Database is returned
2. The Connector (still inside the constructor) sets the connection field to the broken connection that it received from the Databaseclass without checking it
3. PlayerData tries to query the player's data with con.querySQL(...)
4. That method executes PreparedStatement statement = connection.prepareStatement(sql) without doing any null or connection validity checks, causing a SQLException
5. The SQLException is directly caught inside the Connection.querySQL(...) method, it logs "There was a exception with SQL" as error and returns null
6. PlayerData continues to query the rest of the player's data, causing steps 3 - 5 to repeat 6 times
7. PlayerData tries to read the queried data by calling objectiveResults.next(), causing a NullPointerException

Additional note:

It seems that I caused this behavior to change with my request to either call database.getConnection() or refresh() but not both inside the Connector constructor.

With both the database.getConnection() and then the refresh() calls we would end up in the same error that case 🅰️ ends in (the early NullPointerException), but to get there different things would happen:

🅰️ The case that there is no open connection:

The step 2 would be executed twice, causing and logging the same error twice.

🅱️ The case that there already is an open (but dead) connection:

During the Connector.refresh() call:

1. The message "Database connection was lost, reconnecting..." would be logged
2. The connection in Database would get closed:
   • It would probably cause an SQLException that would be caught inside Database.closeConnection() and logged as error with the message "Failed to close the database connection!"
   • The Database.con field would be set to null, causing case 🅰️ to happen on subsequent PlayerData.loadAllPlayerData() calls.
3. The step 2 from 🅰️ would happen once when trying to re-open the database connection
4. The Connector.connection field would be set to null

Cross interference with writing (for the old behavior described in "Additional note");

Given that something was written, then the database became unavailable, then a read happened (triggering the above case 🅱️ behavior) and finally the database is now available again: If we first read something and then try to write the AsyncSaver will still hold onto the dead (and already closed) connection, causing it to reconnect to the database, discarding a good connection in the process, even though a simple database.getConnection() would've been enough.

Originally posted by @seyfahni in #2469 (comment)