diff --git a/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/AbstractDhRepo.java b/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/AbstractDhRepo.java index 8cc6dd76e..7815a15d1 100644 --- a/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/AbstractDhRepo.java +++ b/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/AbstractDhRepo.java @@ -23,8 +23,8 @@ import com.seibel.distanthorizons.core.logging.DhLoggerBuilder; import com.seibel.distanthorizons.core.sql.DatabaseUpdater; import com.seibel.distanthorizons.core.sql.DbConnectionClosedException; import com.seibel.distanthorizons.core.sql.dto.IBaseDTO; +import com.seibel.distanthorizons.core.sql.repo.phantoms.AutoClosableTrackingWrapper; import com.seibel.distanthorizons.core.util.KeyedLockContainer; -import com.seibel.distanthorizons.core.util.ThreadUtil; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; @@ -33,8 +33,7 @@ import java.io.IOException; import java.sql.*; import java.util.*; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.ReentrantLock; /** @@ -53,8 +52,6 @@ public abstract class AbstractDhRepo> implemen private static final ConcurrentHashMap CONNECTIONS_BY_CONNECTION_STRING = new ConcurrentHashMap<>(); private static final ConcurrentHashMap, String> ACTIVE_CONNECTION_STRINGS_BY_REPO = new ConcurrentHashMap<>(); - private static final ThreadPoolExecutor WAL_FLUSH_THREAD = ThreadUtil.makeSingleDaemonThreadPool("Abstract Repo WAL Flush"); - private static final AtomicBoolean FLUSH_THREAD_QUEUED = new AtomicBoolean(false); private final String connectionString; @@ -63,6 +60,8 @@ public abstract class AbstractDhRepo> implemen public final String databaseType; public final File databaseFile; + public final Set openClosables = ConcurrentHashMap.newKeySet(); + public final Class dtoClass; protected final KeyedLockContainer saveLockContainer = new KeyedLockContainer<>(); @@ -356,7 +355,7 @@ public abstract class AbstractDhRepo> implemen // Note: this can only handle 1 command at a time boolean resultSetPresent = statement.execute(sql); - try (ResultSet resultSet = statement.getResultSet()) + try (ResultSet resultSet = AutoClosableTrackingWrapper.wrap(ResultSet.class, statement.getResultSet(), this.openClosables)) { return this.convertResultSetToDictionaryList(resultSet, resultSetPresent); } @@ -405,7 +404,8 @@ public abstract class AbstractDhRepo> implemen boolean resultSetPresent = statement.execute(); if (resultSetPresent) { - return statement.getResultSet(); + ResultSet resultSet = statement.getResultSet(); + return AutoClosableTrackingWrapper.wrap(ResultSet.class, resultSet, this.openClosables); } else { @@ -443,7 +443,7 @@ public abstract class AbstractDhRepo> implemen { PreparedStatement statement = this.connection.prepareStatement(sql); statement.setQueryTimeout(TIMEOUT_SECONDS); - return statement; + return AutoClosableTrackingWrapper.wrap(PreparedStatement.class, statement, this.openClosables); } catch(SQLException e) { @@ -528,6 +528,32 @@ public abstract class AbstractDhRepo> implemen { CONNECTIONS_BY_CONNECTION_STRING.remove(this.connectionString); + + // log any leaked objects + int openClosableCount = this.openClosables.size(); + if (openClosableCount != 0) + { + LOGGER.warn("[" + openClosableCount + "] objects not closed for repo [" + this.getClass().getSimpleName() + "]-[" + this.getTableName() + "] with connection: [" + this.connectionString + "]. A memory leak may be present and closing this connection may take longer than normal."); + + // header + StringBuilder stringBuilder = new StringBuilder(); + stringBuilder.append("Unclosed objects: \n"); + + // leaked objects + HashMap unclosedObjectCountsByString = this.getUnclosedObjectStringsAndCounts(); + for (String objString : unclosedObjectCountsByString.keySet()) + { + AtomicInteger countRef = unclosedObjectCountsByString.get(objString); + if (countRef != null) + { + stringBuilder.append("[" + countRef.get() + "] - [" + objString + "] \n"); + } + } + + LOGGER.warn(stringBuilder.toString()); + } + + if (!this.connection.isClosed()) { LOGGER.info("Closing database connection: [" + this.connectionString + "]..."); @@ -622,6 +648,49 @@ public abstract class AbstractDhRepo> implemen return list; } + /** used for logging leaked objects */ + public HashMap getUnclosedObjectStringsAndCounts() + { + HashMap closableCountsByToString = new HashMap<>(); + + for (AutoClosableTrackingWrapper closableWrapper : this.openClosables) + { + // custom to-strings for better merging + String str = closableWrapper.wrappedClosable.getClass().getSimpleName(); + if (closableWrapper.wrappedClosable instanceof ResultSet) + { + str += " @ " + closableWrapper.wrappedClosable.toString(); + } + else if (closableWrapper.wrappedClosable instanceof PreparedStatement) + { + String sql = closableWrapper.wrappedClosable.toString(); + int parametersIndex = sql.indexOf("\n parameters="); // remove Sqlite parameters so queries aren't separated by properties + if (parametersIndex != -1) + { + sql = sql.substring(0, parametersIndex); + } + str += " @ " + sql; + } + else + { + str += " @ " + closableWrapper.wrappedClosable.toString(); + } + + + closableCountsByToString.compute(str, (stringVal, countRef) -> + { + if (countRef == null) + { + countRef = new AtomicInteger(0); + } + countRef.incrementAndGet(); + return countRef; + }); + } + + return closableCountsByToString; + } + //==================// diff --git a/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/phantoms/AutoClosableTrackingWrapper.java b/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/phantoms/AutoClosableTrackingWrapper.java new file mode 100644 index 000000000..b819f89fa --- /dev/null +++ b/core/src/main/java/com/seibel/distanthorizons/core/sql/repo/phantoms/AutoClosableTrackingWrapper.java @@ -0,0 +1,93 @@ +package com.seibel.distanthorizons.core.sql.repo.phantoms; + +import com.seibel.distanthorizons.coreapi.ModInfo; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.Set; + +/** + * This is used to detect leaks + * with our JDBC implementation, specifically + * to make sure all {@link AutoCloseable} objects + * are inside try-finally resources. + */ +public class AutoClosableTrackingWrapper implements InvocationHandler +{ + //private static final Logger LOGGER = DhLoggerBuilder.getLogger(); + + /** + * should be enabled during development to + * notify if any resources are being leaked. + */ + public static final boolean TRACK_WRAPPERS = ModInfo.IS_DEV_BUILD; + + @NotNull + public final AutoCloseable wrappedClosable; + @NotNull + public final Set parentTrackingSet; + + + + //==============// + // constructors // + //==============// + + @Nullable + public static TStatic wrap( + @NotNull Class clazz, + @Nullable TStatic wrappedClosable, + @NotNull Set trackingSet) + { + if (!TRACK_WRAPPERS) + { + return wrappedClosable; + } + + // done to prevent null pointers + if (wrappedClosable == null) + { + return null; + } + + return (TStatic) Proxy.newProxyInstance( + wrappedClosable.getClass().getClassLoader(), + new Class[]{ clazz }, + new AutoClosableTrackingWrapper(wrappedClosable, trackingSet) + ); + } + + private AutoClosableTrackingWrapper(@NotNull AutoCloseable wrappedClosable, @NotNull Set trackingSet) + { + this.wrappedClosable = wrappedClosable; + this.parentTrackingSet = trackingSet; + this.parentTrackingSet.add(this); + } + + + + //============// + // reflection // + //============// + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable + { + // Track the close() method + if ("close".equals(method.getName())) + { + this.wrappedClosable.close(); + this.parentTrackingSet.remove(this); + return null; + } + + // Delegate all other methods to the wrapped object + return method.invoke(this.wrappedClosable, args); + } + + + +} diff --git a/core/src/test/java/tests/DhRepoSqliteTest.java b/core/src/test/java/tests/DhRepoSqliteTest.java index ea11c172d..8f5e6b877 100644 --- a/core/src/test/java/tests/DhRepoSqliteTest.java +++ b/core/src/test/java/tests/DhRepoSqliteTest.java @@ -19,9 +19,11 @@ package tests; +import com.seibel.distanthorizons.core.logging.DhLoggerBuilder; import com.seibel.distanthorizons.core.pos.DhChunkPos; import com.seibel.distanthorizons.core.sql.DatabaseUpdater; import com.seibel.distanthorizons.core.sql.repo.AbstractDhRepo; +import org.apache.logging.log4j.Logger; import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; @@ -34,13 +36,14 @@ import java.io.File; import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; -import java.util.Map; /** * Validates {@link AbstractDhRepo} is set up correctly. */ public class DhRepoSqliteTest { + private static final Logger LOGGER = DhLoggerBuilder.getLogger(); + public static String DATABASE_TYPE = "jdbc:sqlite"; public static String DB_FILE_NAME = "test.sqlite"; @@ -208,5 +211,128 @@ public class DhRepoSqliteTest } } + /** + * leak detection is done to make sure {@link ResultSet} and {@link PreparedStatement}'s + * are properly cleaned up. + */ + @Test + public void testRepoLeakDetection() + { + TestPrimaryKeyRepo primaryKeyRepo = null; + try + { + primaryKeyRepo = new TestPrimaryKeyRepo(DATABASE_TYPE, new File(DB_FILE_NAME)); + + int insertCount = 10; + int readCount = 10; + + + + Assert.assertEquals(0, primaryKeyRepo.openClosables.size()); + + + //=============================// + // correctly closed statements // + //=============================// + + { + // insert + for (int i = 0; i < insertCount; i++) + { + TestSingleKeyDto insertDto = new TestSingleKeyDto(i, "a", 0L, (byte) 0); + + try (PreparedStatement statement = primaryKeyRepo.createInsertStatement(insertDto)) + { + primaryKeyRepo.query(statement); + + if (i % 1_000 == 0) + { + System.out.println(i + " / " + insertCount); + } + } + } + + Assert.assertEquals("Insert leaks", 0, primaryKeyRepo.openClosables.size()); + + + + // read + TestSingleKeyDto expectedReadDto = new TestSingleKeyDto(1, "a", 0L, (byte) 0); + for (int i = 0; i < readCount; i++) + { + try (PreparedStatement statement = primaryKeyRepo.createSelectStatementByKey(1); + ResultSet resultSet = primaryKeyRepo.query(statement)) + { + TestSingleKeyDto readDto = primaryKeyRepo.convertResultSetToDto(resultSet); + Assert.assertEquals(expectedReadDto.id, readDto.id); + + if (i % 1_000 == 0) + { + System.out.println(i + " / " + readCount); + } + } + } + + Assert.assertEquals("read leaks", 0, primaryKeyRepo.openClosables.size()); + } + + + + //===================// + // leaked statements // + //===================// + { + // nuke the DB so we can insert without worries + primaryKeyRepo.deleteAll(); + + // insert + for (int i = 0; i < insertCount; i++) + { + TestSingleKeyDto insertDto = new TestSingleKeyDto(i, "a", 0L, (byte) 0); + PreparedStatement statement = primaryKeyRepo.createInsertStatement(insertDto); + primaryKeyRepo.query(statement); + + if (i % 1_000 == 0) + { + System.out.println(i + " / " + insertCount); + } + } + + Assert.assertNotEquals(0, primaryKeyRepo.openClosables.size()); + primaryKeyRepo.openClosables.clear(); + + + + // read + for (int i = 0; i < readCount; i++) + { + PreparedStatement statement = primaryKeyRepo.createSelectStatementByKey(1); + ResultSet resultSet = primaryKeyRepo.query(statement); + + TestSingleKeyDto readDto = primaryKeyRepo.convertResultSetToDto(resultSet); + Assert.assertEquals(1, readDto.id); + + if (i % 1_000 == 0) + { + System.out.println(i + " / " + readCount); + } + } + + Assert.assertNotEquals(0, primaryKeyRepo.openClosables.size()); + } + } + catch (SQLException e) + { + Assert.fail(e.getMessage()); + } + finally + { + if (primaryKeyRepo != null) + { + primaryKeyRepo.close(); + } + } + } + }