Require Java 25 (#7305)

Author: labkey-jeckels

api/src/org/labkey/api/data/CachedResultSet.java

@@ -17,6 +17,7 @@
 package org.labkey.api.data;
 
 import org.apache.commons.beanutils.ConvertUtils;
+import java.lang.ref.Cleaner;
 import org.apache.commons.collections4.IteratorUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -25,7 +26,6 @@
 import org.labkey.api.collections.RowMap;
 import org.labkey.api.dataiterator.DataIterator;
 import org.labkey.api.miniprofiler.MiniProfiler;
-import org.labkey.api.settings.AppProps;
 import org.labkey.api.util.ExceptionUtil;
 import org.labkey.api.util.MemTracker;
 import org.labkey.api.util.ResultSetUtil;
@@ -77,27 +77,78 @@ public class CachedResultSet implements ResultSet, TableResultSet
     // data
     private final ArrayList<RowMap<Object>> _rowMaps;
     private final boolean _isComplete;
-    @Nullable
-    private final StackTraceElement[] _stackTrace;
-    private final String _threadName;
 
     private boolean _wasClosed = false;
-    private boolean _requireClose = true;
-    private String _url = null;
 
     // state
     private int _row = -1;
     private int _direction = 1;
     private int _fetchSize = 1;
     private Object _lastObject = null;
 
+    private static final Cleaner CLEANER = Cleaner.create();
+
+    private static class CachedResultSetState implements Runnable
+    {
+        private final boolean _requireClose;
+        private final @Nullable StackTraceElement[] _stackTrace;
+        private final String _threadName;
+        private final String _url;
+        private final Logger _log;
+
+        private boolean _wasClosed = false;
+
+        private CachedResultSetState(boolean requireClose, @Nullable StackTraceElement[] stackTrace, String threadName, String url, Logger log)
+        {
+            _requireClose = requireClose;
+            _stackTrace = stackTrace;
+            _threadName = threadName;
+            _url = url;
+            _log = log;
+        }
+
+        @Override
+        public void run()
+        {
+            if (!_wasClosed)
+            {
+                if (_requireClose)
+                {
+                    StringBuilder error = new StringBuilder("CachedResultSet was not closed.");
+                    if (null != _threadName)
+                    {
+                        error.append(" Created by thread ").append(_threadName);
+                    }
+                    if (null != _url)
+                    {
+                        error.append(" for URL ").append(_url);
+                    }
+                    if (null != _stackTrace)
+                    {
+                        error.append("\n").append(ExceptionUtil.renderStackTrace(_stackTrace));
+                    }
+                    _log.error(error);
+                }
+                _wasClosed = true;
+            }
+        }
+
+        private void close()
+        {
+            _wasClosed = true;
+        }
+    }
+
+    private final CachedResultSetState _state;
+    private final Cleaner.Cleanable _cleanable;
+
 
     /*
         Constructor is not normally used... see CachedResultSets for static factory methods.
 
         stackTrace is used to set an alternate stack trace -- good for async queries, to indicate original creation stack trace
      */
-    CachedResultSet(ResultSetMetaData md, ArrayList<RowMap<Object>> maps, boolean isComplete, @Nullable StackTraceElement[] stackTrace)
+    CachedResultSet(ResultSetMetaData md, ArrayList<RowMap<Object>> maps, boolean isComplete, boolean requireClose, @Nullable StackTraceElement[] stackTrace)
     {
         _rowMaps = maps;
         _isComplete = isComplete;
@@ -120,50 +171,35 @@ public class CachedResultSet implements ResultSet, TableResultSet
             throw new RuntimeSQLException(x);
         }
 
+        String url = null;
+        String threadName = null;
         if (MiniProfiler.isCollectTroubleshootingStackTraces())
         {
             // Stash stack trace that created this CachedRowSet
-            if (null != stackTrace)
+            if (null == stackTrace)
             {
-                _stackTrace = stackTrace;
-            }
-            else
-            {
-                _stackTrace = MiniProfiler.getTroubleshootingStackTrace();
+                stackTrace = MiniProfiler.getTroubleshootingStackTrace();
             }
 
-            _threadName = Thread.currentThread().getName();
+            threadName = Thread.currentThread().getName();
 
             if (HttpView.getStackSize() > 0)
             {
                 try
                 {
-                    _url = ViewServlet.getOriginalURL();
+                    url = ViewServlet.getOriginalURL();
                 }
                 catch (Exception x)
                 {
                     // we might not be in a view thread...
                 }
             }
         }
-        else
-        {
-            _stackTrace = null;
-            _threadName = null;
-        }
 
-        MemTracker.getInstance().put(this);
-    }
-
-    public boolean isRequireClose()
-    {
-        return _requireClose;
-    }
+        _state = new CachedResultSetState(requireClose, stackTrace, threadName, url, _log);
+        _cleanable = CLEANER.register(this, _state);
 
-    public CachedResultSet setRequireClose(boolean requireClose)
-    {
-        _requireClose = requireClose;
-        return this;
+        MemTracker.getInstance().put(this);
     }
 
     @Override
@@ -223,6 +259,8 @@ public boolean next()
     public void close()
     {
         _wasClosed = true;
+        _state.close();
+        _cleanable.clean();
     }
 
     @Override
@@ -729,28 +767,6 @@ public void afterLast()
         _row = _rowMaps.size();
     }
 
-    @Override
-    protected void finalize() throws Throwable
-    {
-        if (!_wasClosed)
-        {
-            close();
-
-            if (_requireClose && AppProps.getInstance().isDevMode())
-            {
-                StringBuilder error = new StringBuilder("CachedResultSet was not closed.");
-                if (null != _url)
-                    error.append("\nURL: ").append(_url);
-                else if (_threadName != null)
-                    error.append("\nthreadName: ").append(_threadName);
-                error.append("\nStack trace from the creation:");
-                error.append(ExceptionUtil.renderStackTrace(_stackTrace));
-
-                _log.error(error);
-            }
-        }
-        super.finalize();
-    }
 
     @Override
     public boolean first()

api/src/org/labkey/api/data/CachedResultSets.java

@@ -32,12 +32,12 @@
  */
 public class CachedResultSets
 {
-    public static CachedResultSet create(ResultSet rs, boolean cacheMetaData, int maxRows) throws SQLException
+    public static CachedResultSet create(ResultSet rs, boolean cacheMetaData, boolean requireClose, int maxRows) throws SQLException
     {
-        return create(rs, cacheMetaData, maxRows, null, QueryLogging.emptyQueryLogging());
+        return create(rs, cacheMetaData, requireClose, maxRows, null, QueryLogging.emptyQueryLogging());
     }
 
-    public static CachedResultSet create(ResultSet rsIn, boolean cacheMetaData, int maxRows, @Nullable StackTraceElement[] stackTrace, QueryLogging queryLogging) throws SQLException
+    public static CachedResultSet create(ResultSet rsIn, boolean cacheMetaData, boolean requireClose, int maxRows, @Nullable StackTraceElement[] stackTrace, QueryLogging queryLogging) throws SQLException
     {
         try (ResultSet rs = new LoggingResultSetWrapper(rsIn, queryLogging))         // TODO: avoid if we're passed a read-only and empty one??
         {
@@ -58,13 +58,13 @@ public static CachedResultSet create(ResultSet rsIn, boolean cacheMetaData, int
             // If we have another row, then we're not complete
             boolean isComplete = !rs.next();
 
-            return new CachedResultSet(md, list, isComplete, stackTrace);
+            return new CachedResultSet(md, list, isComplete, requireClose, stackTrace);
         }
     }
 
     public static CachedResultSet create(ResultSetMetaData md, List<Map<String, Object>> maps, boolean isComplete)
     {
-        return new CachedResultSet(md, convertToRowMaps(md, maps), isComplete, null);
+        return new CachedResultSet(md, convertToRowMaps(md, maps), isComplete, true, null);
     }
 
     public static CachedResultSet create(List<Map<String, Object>> maps)
@@ -88,7 +88,7 @@ public static CachedResultSet create(List<Map<String, Object>> maps, Collection<
         ResultSetMetaData md = createMetaData(columnNames);
 
         // Avoid error message from CachedResultSet.finalize() about unclosed CachedResultSet.
-        try (CachedResultSet crs = new CachedResultSet(md, convertToRowMaps(md, maps), true, null))
+        try (CachedResultSet crs = new CachedResultSet(md, convertToRowMaps(md, maps), true, true, null))
         {
             return crs;
         }

api/src/org/labkey/api/data/ConnectionWrapper.java

@@ -17,6 +17,7 @@
 package org.labkey.api.data;
 
 import org.apache.commons.collections4.multimap.HashSetValuedHashMap;
+import java.lang.ref.Cleaner;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LoggerContext;
@@ -78,6 +79,59 @@ public class ConnectionWrapper implements java.sql.Connection
     private static final Logger packageLogger = LogManager.getLogger(ConnectionWrapper.class.getPackageName());
     private static final Logger LOG = LogHelper.getLogger(ConnectionWrapper.class, "All JDBC metadata and SQL execution calls being made");
 
+    private static final Cleaner CLEANER = Cleaner.create();
+
+    private static class ConnectionState implements Runnable
+    {
+        private Connection _connection;
+        private final DbScope _scope;
+        private final ConnectionWrapper _wrapper; // This is risky, but we need toString()
+        private final Logger _log;
+
+        private boolean _closed = false;
+
+        private ConnectionState(Connection connection, DbScope scope, ConnectionWrapper wrapper, Logger log)
+        {
+            _connection = connection;
+            _scope = scope;
+            _wrapper = wrapper;
+            _log = log;
+        }
+
+        @Override
+        public void run()
+        {
+            if (_connection != null && !_closed)
+            {
+                _log.error("Connection was not closed! " + _wrapper.toString());
+                realClose();
+            }
+        }
+
+        private void realClose()
+        {
+            if (!_closed)
+            {
+                try
+                {
+                    _wrapper.realCloseInternal();
+                }
+                catch (SQLException e)
+                {
+                    _log.error("Failed to close connection", e);
+                }
+                finally
+                {
+                    _closed = true;
+                    _connection = null;
+                }
+            }
+        }
+    }
+
+    private final ConnectionState _state;
+    private final Cleaner.Cleanable _cleanable;
+
     private static final Set<ConnectionWrapper> _openConnections = Collections.synchronizedSet(new HashSet<>());
     private static final Set<ConnectionWrapper> _loggedLeaks = new HashSet<>();
     private static final boolean _explicitLogger = initializeExplicitLogger();
@@ -165,6 +219,8 @@ public ConnectionWrapper(Connection conn, DbScope scope, Integer spid, Connectio
         _openConnections.add(this);
 
         _log = log != null ? log : getConnectionLogger();
+        _state = new ConnectionState(_connection, _scope, this, _log);
+        _cleanable = CLEANER.register(this, _state);
     }
 
     /** this is a best guess logger, pass one in to be predictable */
@@ -457,7 +513,13 @@ void internalClose() throws SQLException
         _type.close(_scope, this, this::realClose);
     }
 
-    private void realClose() throws SQLException
+    private void realClose()
+    {
+        _state.realClose();
+        _cleanable.clean();
+    }
+
+    private void realCloseInternal() throws SQLException
     {
         _openConnections.remove(this);
         _loggedLeaks.remove(this);
@@ -938,18 +1000,6 @@ public boolean isWrapperFor(Class<?> iface)
         return iface.isAssignableFrom(_connection.getClass());
     }
 
-    @Override
-    protected void finalize() throws Throwable
-    {
-        // If the thread was banned from getting a connection, _connection will be null, and we shouldn't complain that it wasn't closed
-        if (_connection != null && !isClosed())
-        {
-            LOG.error("Connection was not closed! " + this);
-            realClose();
-        }
-
-        super.finalize();
-    }
 
     public @NotNull Closer getRunOnClose()
     {

api/src/org/labkey/api/data/DbScope.java

@@ -1878,7 +1878,7 @@ private static void detectUnexpectedConnections(Connection conn, LabKeyDataSourc
             stmt.setString(1, databaseName);
             stmt.setString(2, applicationName);
 
-            try (CachedResultSet rs = CachedResultSets.create(stmt.executeQuery(), true, 1000))
+            try (CachedResultSet rs = CachedResultSets.create(stmt.executeQuery(), true, true, 1000))
             {
                 count = rs.getSize();
                 if (count != 0)