From 2c8bbc80d8c73816323c306b684aa1aa222340c1 Mon Sep 17 00:00:00 2001 From: Risto Lahtela <24460436+AuroraLS3@users.noreply.github.com> Date: Wed, 24 Mar 2021 14:00:08 +0200 Subject: [PATCH] Added a wait for SQLite to finish queries before closing the connection Also: - Transactions now execute as much as possible on the same connection instead of getting a new connection - More shutdown messages when waiting for things (Like SQLite queries, or db transactions). Affects issues: - Possibly fixed #1814 --- .../webserver/auth/ActiveCookieStore.java | 1 - .../plan/settings/locale/lang/PluginLang.java | 4 ++ .../plan/storage/database/SQLDB.java | 4 ++ .../plan/storage/database/SQLiteDB.java | 14 ++++++- .../database/queries/QueryAPIQuery.java | 12 ++++-- .../database/queries/QueryStatement.java | 12 ++++-- .../database/transactions/Transaction.java | 14 ++++++- .../database/transactions/patches/Patch.java | 16 +++++++- .../plan/storage/upkeep/DBKeepAliveTask.java | 10 ++--- .../utilities/SemaphoreAccessCounter.java | 39 +++++++++++++++++++ .../database/DBPatchMySQLRegressionTest.java | 4 +- .../database/DBPatchRegressionTest.java | 5 ++- .../plan/storage/database/DatabaseTest.java | 2 +- 13 files changed, 118 insertions(+), 19 deletions(-) create mode 100644 Plan/common/src/main/java/com/djrapitops/plan/utilities/SemaphoreAccessCounter.java diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/auth/ActiveCookieStore.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/auth/ActiveCookieStore.java index a73957c72..a0383928d 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/auth/ActiveCookieStore.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/auth/ActiveCookieStore.java @@ -71,7 +71,6 @@ public class ActiveCookieStore implements SubSystem { } public static void removeUserCookie(String username) { - System.out.println(USERS_BY_COOKIE); USERS_BY_COOKIE.entrySet().stream().filter(entry -> entry.getValue().getUsername().equals(username)) .findAny() .map(Map.Entry::getKey) diff --git a/Plan/common/src/main/java/com/djrapitops/plan/settings/locale/lang/PluginLang.java b/Plan/common/src/main/java/com/djrapitops/plan/settings/locale/lang/PluginLang.java index 0595e76c5..fca7421ac 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/settings/locale/lang/PluginLang.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/settings/locale/lang/PluginLang.java @@ -58,6 +58,10 @@ public enum PluginLang implements Lang { DISABLED_PROCESSING_COMPLETE("Disable - Processing Complete", "Processing complete."), DISABLED_UNSAVED_SESSIONS("Disable - Unsaved Session Save", "Saving unfinished sessions.."), DISABLED_UNSAVED_SESSIONS_TIMEOUT("Disable - Unsaved Session Save Timeout", "Timeout hit, storing the unfinished sessions on next enable instead."), + DISABLED_WAITING_SQLITE("Disable - Waiting SQLite", "Waiting queries to finish to avoid SQLite crashing JVM.."), + DISABLED_WAITING_SQLITE_COMPLETE("Disable - Waiting SQLite Complete", "Closed SQLite connection."), + DISABLED_WAITING_TRANSACTIONS("Disable - Waiting Transactions", "Waiting for unfinished transactions to avoid data loss.."), + DISABLED_WAITING_TRANSACTIONS_COMPLETE("Disable - Waiting Transactions Complete", "Transaction queue closed."), VERSION_NEWEST("Version - Latest", "You're using the latest version."), VERSION_AVAILABLE("Version - New", "New Release (${0}) is available ${1}"), diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java index a77d5dfcf..8d5d04eee 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java @@ -24,6 +24,7 @@ import com.djrapitops.plan.settings.config.PlanConfig; import com.djrapitops.plan.settings.config.paths.PluginSettings; import com.djrapitops.plan.settings.config.paths.TimeSettings; import com.djrapitops.plan.settings.locale.Locale; +import com.djrapitops.plan.settings.locale.lang.PluginLang; import com.djrapitops.plan.storage.database.queries.Query; import com.djrapitops.plan.storage.database.transactions.Transaction; import com.djrapitops.plan.storage.database.transactions.init.CreateIndexTransaction; @@ -123,6 +124,7 @@ public abstract class SQLDB extends AbstractDatabase { } transactionExecutor.shutdown(); try { + logger.info(locale.getString(PluginLang.DISABLED_WAITING_TRANSACTIONS)); Long waitMs = config.getOrDefault(TimeSettings.DB_TRANSACTION_FINISH_WAIT_DELAY, TimeUnit.SECONDS.toMillis(20L)); if (waitMs > TimeUnit.MINUTES.toMillis(5L)) { logger.warn(TimeSettings.DB_TRANSACTION_FINISH_WAIT_DELAY.getPath() + " was set to over 5 minutes, using 5 min instead."); @@ -138,6 +140,8 @@ public abstract class SQLDB extends AbstractDatabase { } } catch (InterruptedException e) { Thread.currentThread().interrupt(); + } finally { + logger.info(locale.getString(PluginLang.DISABLED_WAITING_TRANSACTIONS_COMPLETE)); } return Collections.emptyList(); } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLiteDB.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLiteDB.java index 7cfe85887..bae68f143 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLiteDB.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLiteDB.java @@ -24,6 +24,7 @@ import com.djrapitops.plan.settings.locale.lang.PluginLang; import com.djrapitops.plan.storage.file.PlanFiles; import com.djrapitops.plan.storage.upkeep.DBKeepAliveTask; import com.djrapitops.plan.utilities.MiscUtils; +import com.djrapitops.plan.utilities.SemaphoreAccessCounter; import com.djrapitops.plan.utilities.logging.ErrorContext; import com.djrapitops.plan.utilities.logging.ErrorLogger; import dagger.Lazy; @@ -49,6 +50,13 @@ public class SQLiteDB extends SQLDB { private Connection connection; private Task connectionPingTask; + /* + * In charge of keeping a single thread in control of the connection to avoid + * one thread closing the connection while another is executing a statement as + * that might lead to a SIGSEGV signal JVM crash. + */ + private final SemaphoreAccessCounter connectionLock = new SemaphoreAccessCounter(); + private SQLiteDB( File databaseFile, Locale locale, @@ -132,6 +140,7 @@ public class SQLiteDB extends SQLDB { if (connection == null) { connection = getNewConnection(databaseFile); } + connectionLock.enter(); return connection; } @@ -140,14 +149,17 @@ public class SQLiteDB extends SQLDB { super.close(); stopConnectionPingTask(); + logger.info(locale.getString(PluginLang.DISABLED_WAITING_SQLITE)); + connectionLock.waitUntilNothingAccessing(); if (connection != null) { MiscUtils.close(connection); } + logger.info(locale.getString(PluginLang.DISABLED_WAITING_SQLITE_COMPLETE)); } @Override public void returnToPool(Connection connection) { - // Connection pool not in use, no action required. + connectionLock.exit(); } @Override diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryAPIQuery.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryAPIQuery.java index ae0d44845..3c9c3f900 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryAPIQuery.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryAPIQuery.java @@ -42,13 +42,19 @@ public class QueryAPIQuery implements Query { Connection connection = null; try { connection = db.getConnection(); - try (PreparedStatement preparedStatement = connection.prepareStatement(sql)) { - return performQuery.apply(preparedStatement); - } + return executeWithConnection(connection); } catch (SQLException e) { throw DBOpException.forCause(sql, e); } finally { db.returnToPool(connection); } } + + public T executeWithConnection(Connection connection) { + try (PreparedStatement preparedStatement = connection.prepareStatement(sql)) { + return performQuery.apply(preparedStatement); + } catch (SQLException e) { + throw DBOpException.forCause(sql, e); + } + } } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryStatement.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryStatement.java index c4e695e0f..06d5d4efc 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryStatement.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/QueryStatement.java @@ -48,9 +48,7 @@ public abstract class QueryStatement implements Query { Connection connection = null; try { connection = db.getConnection(); - try (PreparedStatement preparedStatement = connection.prepareStatement(sql)) { - return executeQuery(preparedStatement); - } + return executeWithConnection(connection); } catch (SQLException e) { throw DBOpException.forCause(sql, e); } finally { @@ -58,6 +56,14 @@ public abstract class QueryStatement implements Query { } } + public T executeWithConnection(Connection connection) { + try (PreparedStatement preparedStatement = connection.prepareStatement(sql)) { + return executeQuery(preparedStatement); + } catch (SQLException e) { + throw DBOpException.forCause(sql, e); + } + } + public T executeQuery(PreparedStatement statement) throws SQLException { try { statement.setFetchSize(fetchSize); diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/Transaction.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/Transaction.java index ffa833906..d960e60f4 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/Transaction.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/Transaction.java @@ -22,6 +22,8 @@ import com.djrapitops.plan.storage.database.DBType; import com.djrapitops.plan.storage.database.Database; import com.djrapitops.plan.storage.database.SQLDB; import com.djrapitops.plan.storage.database.queries.Query; +import com.djrapitops.plan.storage.database.queries.QueryAPIQuery; +import com.djrapitops.plan.storage.database.queries.QueryStatement; import com.djrapitops.plan.utilities.logging.ErrorContext; import net.playeranalytics.plugin.scheduling.TimeAmount; @@ -194,7 +196,13 @@ public abstract class Transaction { } protected T query(Query query) { - return query.executeQuery(db); + if (query instanceof QueryStatement) { + return ((QueryStatement) query).executeWithConnection(connection); + } else if (query instanceof QueryAPIQuery) { + return ((QueryAPIQuery) query).executeWithConnection(connection); + } else { + return query.executeQuery(db); + } } protected boolean execute(Executable executable) { @@ -226,7 +234,9 @@ public abstract class Transaction { transaction.db = db; transaction.dbType = dbType; transaction.connection = this.connection; - transaction.performOperations(); + if (transaction.shouldBeExecuted()) { + transaction.performOperations(); + } transaction.connection = null; transaction.dbType = null; transaction.db = null; diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java index 8f8b5e2d7..c32e57e95 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java @@ -34,20 +34,34 @@ import static com.djrapitops.plan.storage.database.sql.building.Sql.*; public abstract class Patch extends OperationCriticalTransaction { private static final String ALTER_TABLE = "ALTER TABLE "; + private boolean appliedPreviously = false; + private boolean appliedNow = false; public abstract boolean hasBeenApplied(); protected abstract void applyPatch(); + public boolean isApplied() { + if (!success) throw new IllegalStateException("Asked a Patch if it is applied before it was executed!"); + return appliedPreviously || appliedNow; + } + + public boolean wasApplied() { + return appliedNow; + } + @Override protected boolean shouldBeExecuted() { - return !hasBeenApplied(); + boolean hasBeenApplied = hasBeenApplied(); + if (hasBeenApplied) appliedPreviously = true; + return !hasBeenApplied; } @Override protected void performOperations() { if (dbType == DBType.MYSQL) disableForeignKeyChecks(); applyPatch(); + appliedNow = true; if (dbType == DBType.MYSQL) enableForeignKeyChecks(); } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/upkeep/DBKeepAliveTask.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/upkeep/DBKeepAliveTask.java index 1303004a0..76d096988 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/upkeep/DBKeepAliveTask.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/upkeep/DBKeepAliveTask.java @@ -33,14 +33,14 @@ import java.sql.Statement; * @author Fuzzlemann */ public class DBKeepAliveTask extends PluginRunnable { - private final IReconnect iReconnect; + private final Reconnector reconnector; private final PluginLogger logger; private final ErrorLogger errorLogger; private Connection connection; - public DBKeepAliveTask(Connection connection, IReconnect iReconnect, PluginLogger logger, ErrorLogger errorLogger) { + public DBKeepAliveTask(Connection connection, Reconnector reconnector, PluginLogger logger, ErrorLogger errorLogger) { this.connection = connection; - this.iReconnect = iReconnect; + this.reconnector = reconnector; this.logger = logger; this.errorLogger = errorLogger; } @@ -56,7 +56,7 @@ public class DBKeepAliveTask extends PluginRunnable { } } catch (SQLException pingException) { try { - connection = iReconnect.reconnect(); + connection = reconnector.reconnect(); } catch (SQLException reconnectionError) { errorLogger.error(reconnectionError, ErrorContext.builder() .whatToDo("Reload Plan and Report this if the issue persists").build()); @@ -68,7 +68,7 @@ public class DBKeepAliveTask extends PluginRunnable { } } - public interface IReconnect { + public interface Reconnector { Connection reconnect() throws SQLException; } } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/utilities/SemaphoreAccessCounter.java b/Plan/common/src/main/java/com/djrapitops/plan/utilities/SemaphoreAccessCounter.java new file mode 100644 index 000000000..f0f06c33a --- /dev/null +++ b/Plan/common/src/main/java/com/djrapitops/plan/utilities/SemaphoreAccessCounter.java @@ -0,0 +1,39 @@ +package com.djrapitops.plan.utilities; + +import java.util.concurrent.atomic.AtomicInteger; + +public class SemaphoreAccessCounter { + + private final AtomicInteger accessCounter; + private final Object lockObject; + + public SemaphoreAccessCounter() { + accessCounter = new AtomicInteger(0); + lockObject = new Object(); + } + + public void enter() { + accessCounter.incrementAndGet(); + } + + public void exit() { + synchronized (lockObject) { + int value = accessCounter.decrementAndGet(); + if (value == 0) { + lockObject.notifyAll(); + } + } + } + + public void waitUntilNothingAccessing() { + while (accessCounter.get() > 0) { + synchronized (lockObject) { + try { + lockObject.wait(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + } + } +} diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchMySQLRegressionTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchMySQLRegressionTest.java index 10d29b7c8..58b4fa874 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchMySQLRegressionTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchMySQLRegressionTest.java @@ -33,6 +33,7 @@ import utilities.mocks.PluginMockComponent; import java.nio.file.Path; import java.util.Optional; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -152,6 +153,7 @@ class DBPatchMySQLRegressionTest extends DBPatchRegressionTest { KillsOptimizationPatch patch = new KillsOptimizationPatch(); underTest.executeTransaction(patch); - assertTrue(patch.hasBeenApplied()); + assertTrue(patch.isApplied()); + assertFalse(patch.wasApplied()); } } diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchRegressionTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchRegressionTest.java index f8e97adde..d704888f1 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchRegressionTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DBPatchRegressionTest.java @@ -86,8 +86,11 @@ abstract class DBPatchRegressionTest { void assertPatchesHaveBeenApplied(Patch[] patches) { List failed = new ArrayList<>(); for (Patch patch : patches) { - if (!patch.hasBeenApplied()) { + if (!patch.isApplied()) { + System.out.println("! NOT APPLIED: " + patch.getClass().getSimpleName()); failed.add(patch.getClass().getSimpleName()); + } else { + System.out.println(" WAS APPLIED: " + patch.getClass().getSimpleName()); } } assertTrue(failed.isEmpty(), "Patches " + failed + " were not applied properly."); diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DatabaseTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DatabaseTest.java index f3f5e233b..4e71192b2 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DatabaseTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/DatabaseTest.java @@ -292,7 +292,7 @@ public interface DatabaseTest extends DatabaseTestPreparer { // Test expected result Optional updatedBaseUser = db().query(BaseUserQueries.fetchBaseUserOfPlayer(playerUUID)); assertEquals(0L, updatedBaseUser.isPresent() ? updatedBaseUser.get().getRegistered() : null); - assertTrue(testedPatch.hasBeenApplied()); + assertTrue(testedPatch.isApplied()); } @Test