You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
PlayerData.loadAllPlayerData() is called.
That method creates a local variable: Connector con = new Connector()
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:
The connection is opened by the Database: con = openConnection()
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
No error reaches getConnection() of the Database class, it's con class field is set to null and it returns null
The Connector (still inside the constructor) sets the connection field to null that it received from the Databaseclass
PlayerData tries to query the player's data with con.querySQL(...)
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:
The dead connection still stored in Database is returned
The Connector (still inside the constructor) sets the connection field to the broken connection that it received from the Databaseclass without checking it
PlayerData tries to query the player's data with con.querySQL(...)
That method executes PreparedStatement statement = connection.prepareStatement(sql) without doing any null or connection validity checks, causing a SQLException
The SQLException is directly caught inside the Connection.querySQL(...) method, it logs "There was a exception with SQL" as error and returns null
PlayerData continues to query the rest of the player's data, causing steps 3 - 5 to repeat 6 times
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:
The message "Database connection was lost, reconnecting..." would be logged
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.
The step 2 from 🅰️ would happen once when trying to re-open the database connection
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.
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:
PlayerData.loadAllPlayerData()
is called.Connector con = new Connector()
Connector
the database connection fetched from theDatabase
instance stored by the plugin:connection = database.getConnection();
There are two cases:
Database
:con = openConnection()
null
getConnection()
of theDatabase
class, it'scon
class field is set tonull
and it returnsnull
Connector
(still inside the constructor) sets theconnection
field tonull
that it received from theDatabase
classPlayerData
tries to query the player's data withcon.querySQL(...)
PreparedStatement statement = connection.prepareStatement(sql)
without doing any null or connection validity checks, causing aNullPointerException
Database
is returnedConnector
(still inside the constructor) sets theconnection
field to the broken connection that it received from theDatabase
class without checking itPlayerData
tries to query the player's data withcon.querySQL(...)
PreparedStatement statement = connection.prepareStatement(sql)
without doing any null or connection validity checks, causing aSQLException
SQLException
is directly caught inside theConnection.querySQL(...)
method, it logs "There was a exception with SQL" as error and returnsnull
PlayerData
continues to query the rest of the player's data, causing steps 3 - 5 to repeat 6 timesPlayerData
tries to read the queried data by callingobjectiveResults.next()
, causing aNullPointerException
Additional note:
It seems that I caused this behavior to change with my request to either call
database.getConnection()
orrefresh()
but not both inside theConnector
constructor.With both the🅰️ ends in (the early
database.getConnection()
and then therefresh()
calls we would end up in the same error that caseNullPointerException
), but to get there different things would happen:The step 2 would be executed twice, causing and logging the same error twice.
During the
Connector.refresh()
call:SQLException
that would be caught insideDatabase.closeConnection()
and logged as error with the message "Failed to close the database connection!"Database.con
field would be set tonull
, causing casePlayerData.loadAllPlayerData()
calls.Connector.connection
field would be set tonull
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 simpledatabase.getConnection()
would've been enough.Originally posted by @seyfahni in #2469 (comment)
The text was updated successfully, but these errors were encountered: