This is an automated email from the ASF dual-hosted git repository.
paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 5b75e21feb GROOVY-12371: bound the Sql statement cache and make it
thread-safe
5b75e21feb is described below
commit 5b75e21feb99a5676f24c3dde932af1ffae9a578
Author: Paul King <[email protected]>
AuthorDate: Sun Sep 6 15:34:05 2026 +1000
GROOVY-12371: bound the Sql statement cache and make it thread-safe
The prepared-statement cache was an unbounded HashMap keyed on the SQL
text, mutated from getAbstractStatement without synchronization. Two
problems followed. It grew without limit whenever the text varied rather
than the parameters — an inList expands to a different placeholder count
per list size, so a caller sizing the list mints a distinct cached
statement each time, held open until the Sql is closed. And a shared Sql
using the cache from more than one thread could corrupt the map.
The cache is now a synchronized, access-ordered LinkedHashMap that evicts
the least recently used statement past a cap, closing it as it goes so a
bounded cache does not leak the cursor it drops. The cap is
statementCacheSize, default 256, settable and overridable with the
groovy.sql.statement.cache.size system property; 0 or less keeps the old
unbounded behaviour for anyone who relied on it.
Statement creation stays outside the lock — only the get-then-put is
synchronized — so preparing a statement does not serialize other callers,
and a create race keeps the first result and closes the loser rather than
leaking it.
One consequence to note: with a shared Sql caching across threads, a bound
below the working set can now evict and close a statement another thread is
about to reuse. A single Sql is typically used serially or per connection;
where it is shared, size the cache above the working set (or leave it
unbounded). Default 256 is generous enough that ordinary use never evicts.
---
.../groovy-sql/src/main/java/groovy/sql/Sql.java | 96 ++++++++++++++++++----
.../src/test/groovy/groovy/sql/SqlCacheTest.groovy | 37 +++++++++
2 files changed, 117 insertions(+), 16 deletions(-)
diff --git a/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
b/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
index 9e8bb03fc4..309e3e4ac5 100644
--- a/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
+++ b/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
@@ -44,6 +44,7 @@ import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
+import java.util.LinkedHashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
@@ -314,7 +315,28 @@ public class Sql implements AutoCloseable {
private boolean withinBatch;
- private final Map<String, Statement> statementCache = new HashMap<>();
+ /**
+ * Upper bound on the number of prepared statements kept in {@link
#statementCache}. A value
+ * of {@code 0} or less means unbounded, the historical behaviour.
Defaults to 256, overridable
+ * with the {@code groovy.sql.statement.cache.size} system property.
+ */
+ private int statementCacheSize =
Integer.getInteger("groovy.sql.statement.cache.size", 256);
+
+ // Access-ordered so eviction is LRU, and synchronized so a shared Sql
does not corrupt the map;
+ // the eldest entry is closed as it is evicted, otherwise a bounded cache
would leak the JDBC
+ // cursor it drops. SQL whose *text* varies — an inList expanding to a
different placeholder
+ // count per call — would otherwise grow this without limit.
+ private final Map<String, Statement> statementCache =
Collections.synchronizedMap(
+ new LinkedHashMap<String, Statement>(16, 0.75f, true) {
+ @Override
+ protected boolean removeEldestEntry(Map.Entry<String,
Statement> eldest) {
+ if (statementCacheSize > 0 && size() > statementCacheSize)
{
+ closeStatementQuietly(eldest.getValue());
+ return true;
+ }
+ return false;
+ }
+ });
private final Map<String, String> namedParamSqlCache = new HashMap<>();
private final Map<String, List<Tuple<?>>> namedParamIndexPropCache = new
HashMap<>();
private List<String> keyColumnNames;
@@ -4047,6 +4069,33 @@ public class Sql implements AutoCloseable {
}
}
+ /**
+ * The maximum number of statements kept when {@code cacheStatements} is
on. A value of 0 or
+ * less means unbounded. Defaults to 256, or the {@code
groovy.sql.statement.cache.size} system
+ * property. A cache keyed on the SQL text grows once per distinct text,
so a query whose text
+ * varies — an {@code inList} expanding to a different placeholder count
per call — will fill
+ * an unbounded cache; the bound keeps that in check by evicting and
closing the least recently
+ * used statement.
+ *
+ * @return the cache size limit
+ * @since 6.0.0
+ */
+ public int getStatementCacheSize() {
+ return statementCacheSize;
+ }
+
+ /**
+ * Sets the maximum number of statements kept when {@code cacheStatements}
is on; see
+ * {@link #getStatementCacheSize()}. Lowering it below the current number
of cached statements
+ * takes effect as statements are next used and evicted, not immediately.
+ *
+ * @param statementCacheSize the new limit; 0 or less for unbounded
+ * @since 6.0.0
+ */
+ public void setStatementCacheSize(int statementCacheSize) {
+ this.statementCacheSize = statementCacheSize;
+ }
+
/**
* @return boolean true if cache is enabled (default is false)
*/
@@ -5035,20 +5084,25 @@ public class Sql implements AutoCloseable {
private void clearStatementCache() {
Statement[] statements;
- if (statementCache.isEmpty())
- return;
- statements = new Statement[statementCache.size()];
- statementCache.values().toArray(statements);
- statementCache.clear();
+ synchronized (statementCache) {
+ if (statementCache.isEmpty())
+ return;
+ statements = statementCache.values().toArray(new Statement[0]);
+ statementCache.clear();
+ }
for (Statement s : statements) {
- try {
- s.close();
- } catch (Exception e) {
- // It's normally safe to ignore exceptions during cleanup but
here if there is
- // a closed statement in the cache, the cache is possibly
corrupted, hence log
- // at slightly elevated level than similar cases.
- LOG.info("Failed to close statement. Already closed? Exception
message: " + e.getMessage());
- }
+ closeStatementQuietly(s);
+ }
+ }
+
+ private static void closeStatementQuietly(Statement s) {
+ try {
+ if (s != null) s.close();
+ } catch (Exception e) {
+ // It's normally safe to ignore exceptions during cleanup but here
if there is
+ // a closed statement in the cache, the cache is possibly
corrupted, hence log
+ // at slightly elevated level than similar cases.
+ LOG.info("Failed to close statement. Already closed? Exception
message: " + e.getMessage());
}
}
@@ -5057,8 +5111,18 @@ public class Sql implements AutoCloseable {
if (cacheStatements) {
stmt = statementCache.get(sql);
if (stmt == null) {
- stmt = cmd.execute(connection, sql);
- statementCache.put(sql, stmt);
+ Statement created = cmd.execute(connection, sql);
+ synchronized (statementCache) {
+ Statement raced = statementCache.get(sql);
+ if (raced != null) {
+ // another thread prepared the same SQL first; keep
theirs, drop ours
+ closeStatementQuietly(created);
+ stmt = raced;
+ } else {
+ statementCache.put(sql, created);
+ stmt = created;
+ }
+ }
}
} else {
stmt = cmd.execute(connection, sql);
diff --git
a/subprojects/groovy-sql/src/test/groovy/groovy/sql/SqlCacheTest.groovy
b/subprojects/groovy-sql/src/test/groovy/groovy/sql/SqlCacheTest.groovy
index 6b31f3d946..46e31df763 100644
--- a/subprojects/groovy-sql/src/test/groovy/groovy/sql/SqlCacheTest.groovy
+++ b/subprojects/groovy-sql/src/test/groovy/groovy/sql/SqlCacheTest.groovy
@@ -118,6 +118,43 @@ class SqlCacheTest extends GroovyTestCase {
}
}
+ // GROOVY-12371: the statement cache is keyed on SQL text and was
unbounded, so text that
+ // varies — an inList expanding to a different placeholder count, say —
grew it without limit.
+ // It is now capped, evicting (and closing) the least recently used
statement.
+ void testStatementCacheIsBoundedAndEvictsLeastRecentlyUsed() {
+ sql.cacheStatements = true
+ sql.statementCacheSize = 2
+ assert sql.statementCacheSize == 2
+
+ prepareStatementCallCounter = 0
+ sql.firstRow("select * from PERSON where id = ?", [1])
// prepare A, cache [A]
+ sql.firstRow("select * from PERSON where id = ? or id = ?", [1, 2])
// prepare B, cache [A,B]
+ sql.firstRow("select * from PERSON where id > ?", [0])
// prepare C -> evict A, cache [B,C]
+ int afterThreeDistinct = prepareStatementCallCounter
+ assert afterThreeDistinct == 3
+
+ // B and C are still cached: re-running them prepares nothing new
+ sql.firstRow("select * from PERSON where id = ? or id = ?", [1, 2])
+ sql.firstRow("select * from PERSON where id > ?", [0])
+ assert prepareStatementCallCounter == afterThreeDistinct
+
+ // A was evicted, so it must be prepared again
+ sql.firstRow("select * from PERSON where id = ?", [1])
+ assert prepareStatementCallCounter == afterThreeDistinct + 1
+ }
+
+ void testStatementCacheUnboundedWhenSizeNotPositive() {
+ sql.cacheStatements = true
+ sql.statementCacheSize = 0 // unbounded, the historical behaviour
+
+ prepareStatementCallCounter = 0
+ (1..20).each { n -> sql.firstRow("select * from PERSON where id = ? /*
${n} */".toString(), [1]) }
+ int prepared = prepareStatementCallCounter
+ // every one is retained, so re-running the first prepares nothing new
+ sql.firstRow("select * from PERSON where id = ? /* 1 */", [1])
+ assert prepareStatementCallCounter == prepared
+ }
+
void testCachePreparedStatements() {
prepareStatementCallCounter = 0
prepareStatementExpectedCall = 3
