This is an automated email from the ASF dual-hosted git repository.
borinquenkid pushed a commit to branch
fix/flaky-where-query-closure-capture-cache
in repository https://gitbox.apache.org/repos/asf/grails-core.git
The following commit(s) were added to
refs/heads/fix/flaky-where-query-closure-capture-cache by this push:
new 0d195f1493 Address review: skip caching interned nodes, compute
outside lock, fix isDomainClass javadoc claim
0d195f1493 is described below
commit 0d195f1493d136ab3f69e61aecf36b7ccaf5eac0
Author: Walter Duque de Estrada <[email protected]>
AuthorDate: Mon Aug 17 08:59:47 2026 -0500
Address review: skip caching interned nodes, compute outside lock, fix
isDomainClass javadoc claim
Responds to jdaugherty's latest review round on AstPropertyResolveUtils:
- The per-ClassNode property cache does not make isDomainClass
collision-free:
AstUtils#isDomainClass(ClassNode) is @Memoized by Groovy's default,
equality-keyed memoize, and ClassNode#equals()/hashCode() reduce to the
class
name. Corrected the javadoc's "no collision is possible" claim to scope
it to
the property map and call out this separate, pre-existing exposure
instead of
overclaiming immunity to it. Restored the "must stay unique" wording in
WhereQueryClosureCaptureSpec/WhereQueryEmbeddedBlockTransformSpec
accordingly.
- getPropertiesFromCache now skips caching for non-primary ClassNodes
(ClassNode#isPrimaryClassNode() == false), which is what interned,
JVM-wide-shared singletons like ClassHelper.OBJECT_TYPE/STRING_TYPE are.
QueryStringTransformer can feed such a node in as a cache holder for a
JDK-typed property; this class no longer writes its metadata key into a
node
it doesn't own. These nodes are cheap to recompute, so skipping the cache
for
them has no material cost.
- computeProperties can force a domain class's static initializer and invoke
user-written static getters via ClassPropertyFetcher. Moved that call
outside
the synchronized(cacheHolder) block so the per-node monitor is only held
to
check for and publish the result, not while running that code.
- IDENTITY/VERSION property types now use
ClassHelper.make(Long.class).getPlainNodeReference(), matching the
existing
hasMany/belongsTo/hasOne convention, instead of a fresh, uninterned
new ClassNode(Long.class) per computation.
- Fixed a no-op assertion in AstPropertyResolveUtilsSpec: a bare boolean
expression in an "and:" block continuing "given:" is never implicitly
asserted by Spock. Added an explicit assert.
- Added cleanup: blocks to the concurrency tests (executor.shutdownNow(),
timed barrier.await()) and closed the GroovyClassLoader in the reflection
test, so a hung/failed thread can no longer leak a non-daemon thread pool
into the rest of the test JVM (forkEvery = 50).
Verified: :grails-datamapping-core:test full module suite passes, including
all three touched specs. codeStyle (Checkstyle + CodeNarc) reports no
violations across the whole repo.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
---
.../gorm/transform/AstPropertyResolveUtils.java | 53 ++++++++++++++++++++--
.../transform/WhereQueryClosureCaptureSpec.groovy | 18 ++++----
.../WhereQueryEmbeddedBlockTransformSpec.groovy | 16 ++++---
.../transform/AstPropertyResolveUtilsSpec.groovy | 21 ++++++---
4 files changed, 82 insertions(+), 26 deletions(-)
diff --git
a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java
b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java
index 3ea291deb6..4778217fae 100644
---
a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java
+++
b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java
@@ -49,7 +49,8 @@ import org.grails.datastore.mapping.reflect.NameUtils;
public class AstPropertyResolveUtils {
/**
- * Key under which the resolved property map is stashed via {@link
ClassNode#getNodeMetaData(Object, java.util.function.Function)}.
+ * Key under which the resolved property map is stashed as {@code
ClassNode} metadata via
+ * {@link #getPropertiesFromCache}.
* <p>
* Earlier versions of this class cached resolved properties in a single
static, process-wide
* {@code Map} keyed by class name (later by {@code ClassNode} identity).
Both designs share a
@@ -65,12 +66,21 @@ public class AstPropertyResolveUtils {
* <li>No collision is possible between distinct {@code ClassNode}
instances that happen to
* share a name (e.g. classes compiled without a package, or the same
source compiled twice
* in separate {@code GroovyClassLoader}s) - each instance owns its
own metadata storage, so
- * there is no shared key space to collide on in the first place.</li>
+ * there is no shared key space to collide on in the first place. This
holds for the resolved
+ * <em>property map</em> built here.</li>
* <li>No leak is possible - the cached data is only reachable through
the {@code ClassNode}
* it describes, so it becomes eligible for garbage collection at the
same time as the node
* (and the compilation/classloader it belongs to) rather than being
pinned forever by a
* static field.</li>
* </ul>
+ * This does <strong>not</strong> extend to whether a given {@code
ClassNode} is treated as a
+ * domain class in the first place: {@link
AstUtils#isDomainClass(ClassNode)} is
+ * {@code @Memoized}, and Groovy's default memoize is keyed by argument
<em>equality</em>
+ * ({@code ClassNode#equals}/{@code hashCode} both reduce to the class
name), not identity. Two
+ * distinct, same-named {@code ClassNode}s can therefore still receive the
same domain-class
+ * verdict from each other via that memo cache, independent of the
property cache here. That is
+ * a narrower, pre-existing exposure in {@code isDomainClass} itself -
this class neither causes
+ * nor fixes it, and it is tracked separately rather than folded into this
cache's guarantees.
* Per {@link ClassNode#getModule()}'s own convention, the cache is stored
on
* {@link ClassNode#redirect()} - the node a
placeholder/generics-parameterized reference
* ultimately stands in for - so that looking a class up through different
reference nodes still
@@ -116,6 +126,24 @@ public class AstPropertyResolveUtils {
* metadata key to the same shared node concurrently without also
synchronizing on that node -
* this class has no way to compel that. That residual risk belongs to
{@code ClassNode}'s
* metadata storage in general, not to anything specific to the cache here.
+ * <p>
+ * Those interned singleton nodes ({@link ClassNode#isPrimaryClassNode()}
returns {@code false}
+ * for them, since {@link ClassHelper#make(Class)} builds them via the
{@code ClassNode(Class)}
+ * constructor) are never written to by this class at all: {@link
#getPropertiesFromCache}
+ * recomputes for them on every call instead of caching. Caching there
would mean writing this
+ * class's own metadata key into a node no single compilation owns,
growing the residual risk
+ * above from "someone else's key might race with a read" to "this class's
own writes pollute a
+ * JVM-wide-shared node forever" - not a real cost, since the properties
of a JDK type are cheap
+ * to recompute and there are only a handful of these nodes in practice.
+ * <p>
+ * {@link #computeProperties} can run arbitrary user code for a resolved
domain class - it
+ * forces the class's static initializer and invokes user-written static
getters via
+ * {@link ClassPropertyFetcher}. Running that while holding the per-node
monitor would risk
+ * deadlocking against unrelated code the caller has no visibility into, so
+ * {@link #getPropertiesFromCache} computes outside the lock and only
takes the monitor to check
+ * for and, if needed, publish the result. Two threads can therefore both
compute for the same
+ * as-yet-uncached node; the second to acquire the monitor discards its
own (equivalent) result
+ * and returns whatever the first published, so only one map is ever
visible to callers.
*/
private static final String PROPERTIES_CACHE_KEY =
AstPropertyResolveUtils.class.getName() + ".properties";
@@ -165,8 +193,23 @@ public class AstPropertyResolveUtils {
private static Map<String, ClassNode> getPropertiesFromCache(ClassNode
classNode) {
ClassNode cacheHolder = classNode.redirect();
+ if (!cacheHolder.isPrimaryClassNode()) {
+ return computeProperties(cacheHolder);
+ }
synchronized (cacheHolder) {
- return cacheHolder.getNodeMetaData(PROPERTIES_CACHE_KEY, key ->
computeProperties(cacheHolder));
+ Map<String, ClassNode> cached =
cacheHolder.getNodeMetaData(PROPERTIES_CACHE_KEY);
+ if (cached != null) {
+ return cached;
+ }
+ }
+ Map<String, ClassNode> computed = computeProperties(cacheHolder);
+ synchronized (cacheHolder) {
+ Map<String, ClassNode> cached =
cacheHolder.getNodeMetaData(PROPERTIES_CACHE_KEY);
+ if (cached != null) {
+ return cached;
+ }
+ cacheHolder.putNodeMetaData(PROPERTIES_CACHE_KEY, computed);
+ return computed;
}
}
@@ -174,8 +217,8 @@ public class AstPropertyResolveUtils {
Map<String, ClassNode> newProperties = new HashMap<>();
boolean isDomainClass = AstUtils.isDomainClass(classNode);
if (isDomainClass) {
- newProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
- newProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.IDENTITY,
ClassHelper.make(Long.class).getPlainNodeReference());
+ newProperties.put(GormProperties.VERSION,
ClassHelper.make(Long.class).getPlainNodeReference());
}
ClassNode currentNode = classNode;
while (currentNode != null &&
!currentNode.equals(ClassHelper.OBJECT_TYPE)) {
diff --git
a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryClosureCaptureSpec.groovy
b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryClosureCaptureSpec.groovy
index 31ee016851..240c372779 100644
---
a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryClosureCaptureSpec.groovy
+++
b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryClosureCaptureSpec.groovy
@@ -31,14 +31,16 @@ import spock.lang.Specification
*/
class WhereQueryClosureCaptureSpec extends Specification {
- // Historical note: these domain class names were made unique across the
test JVM
- // (ClosureCaptureBook/ClosureCaptureAuthor rather than the more generic
Book/Author)
- // because AstPropertyResolveUtils used to cache resolved properties in a
single
- // static map keyed by class name, so a same-named fixture in another spec
could
- // collide with this one. AstPropertyResolveUtils now caches per-ClassNode
instance
- // (see its javadoc), so that collision can no longer happen regardless of
naming -
- // the unique names are kept only because they make the fixture's purpose
clearer,
- // not because uniqueness is required for correctness.
+ // These domain class names (ClosureCaptureBook/ClosureCaptureAuthor
rather than the more
+ // generic Book/Author) must stay unique across the test JVM.
AstPropertyResolveUtils caches
+ // resolved properties per-ClassNode instance (see its javadoc), which is
enough to stop two
+ // same-named-but-distinct ClassNodes from corrupting each other's
property maps. It does not
+ // cover AstUtils#isDomainClass(ClassNode): that check is @Memoized by
Groovy's default,
+ // equality-keyed memoize, and ClassNode#equals()/hashCode() reduce to the
class name - so a
+ // same-named, differently-shaped fixture compiled in another spec sharing
this JVM fork
+ // (forkEvery = 50) can still hand this class a stale domain-class verdict
from that memo
+ // cache, regardless of the property-map fix. Renaming these back to
Book/Author would risk
+ // exactly that order-dependent flake again.
private static final String SERVICE_SOURCE = '''
import grails.gorm.DetachedCriteria
import grails.gorm.annotation.Entity
diff --git
a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryEmbeddedBlockTransformSpec.groovy
b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryEmbeddedBlockTransformSpec.groovy
index 8b2660ad9d..0bed29a7e7 100644
---
a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryEmbeddedBlockTransformSpec.groovy
+++
b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryEmbeddedBlockTransformSpec.groovy
@@ -33,13 +33,15 @@ import spock.lang.Specification
*/
class WhereQueryEmbeddedBlockTransformSpec extends Specification {
- // Historical note: these domain class names were made unique across the
test JVM
- // because AstPropertyResolveUtils used to cache resolved properties in a
single
- // static map keyed by class name, so a same-named fixture in another spec
could
- // collide with this one. AstPropertyResolveUtils now caches per-ClassNode
instance
- // (see its javadoc), so that collision can no longer happen regardless of
naming -
- // the distinctive names are kept only because they make the fixture's
purpose
- // clearer, not because uniqueness is required for correctness.
+ // These domain class names must stay unique across the test JVM.
AstPropertyResolveUtils
+ // caches resolved properties per-ClassNode instance (see its javadoc),
which is enough to
+ // stop two same-named-but-distinct ClassNodes from corrupting each
other's property maps. It
+ // does not cover AstUtils#isDomainClass(ClassNode): that check is
@Memoized by Groovy's
+ // default, equality-keyed memoize, and ClassNode#equals()/hashCode()
reduce to the class name
+ // - so a same-named, differently-shaped fixture compiled in another spec
sharing this JVM
+ // fork (forkEvery = 50) can still hand this class a stale domain-class
verdict from that memo
+ // cache, regardless of the property-map fix. Reusing a generic name would
risk exactly that
+ // order-dependent flake again.
private static final String SERVICE_SOURCE = '''
import grails.gorm.DetachedCriteria
import grails.gorm.annotation.Entity
diff --git
a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy
b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy
index 4ddc7a6204..d1b6dd9cd3 100644
---
a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy
+++
b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy
@@ -95,8 +95,10 @@ class AstPropertyResolveUtilsSpec extends Specification {
// these are two unrelated ClassNode instances with different declared
properties. A cache
// keyed by name or by equals()/hashCode() would treat them as the
same entry; only
// reference identity (!first.is(second)) tells them apart, which is
exactly what the
- // cache must key on.
- !first.is(second)
+ // cache must key on. This is an "and:" continuing "given:", so Spock
does not apply an
+ // implicit condition here - the explicit assert is required for this
to actually fail
+ // the test if it were ever untrue.
+ assert !first.is(second)
when: 'both class nodes are resolved'
List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
@@ -202,6 +204,9 @@ class AstPropertyResolveUtilsSpec extends Specification {
then: 'the reflected association properties are present alongside the
injected identity/version properties'
propertyNames.containsAll([GormProperties.IDENTITY,
GormProperties.VERSION, 'books', 'author', 'publisher'])
+
+ cleanup:
+ gcl.close()
}
void "concurrent resolution of distinct, identically-named ClassNode
instances never corrupts each other's cached properties"() {
@@ -213,7 +218,7 @@ class AstPropertyResolveUtilsSpec extends Specification {
when: 'all threads race to populate the cache for their own instance
at the same time'
List<Future<Boolean>> futures = (0..<threadCount).collect { int i ->
executor.submit({ ->
- barrier.await()
+ barrier.await(30, TimeUnit.SECONDS)
ClassNode node = new ClassNode('ConcurrentWidget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
String propertyName = "prop${i}".toString()
node.addProperty(propertyName, Modifier.PUBLIC,
ClassHelper.STRING_TYPE, null, null, null)
@@ -223,10 +228,12 @@ class AstPropertyResolveUtilsSpec extends Specification {
} as Callable<Boolean>)
}
List<Boolean> outcomes = futures.collect { Future<Boolean> future ->
future.get(30, TimeUnit.SECONDS) }
- executor.shutdown()
then: 'every thread resolved its own property set, uncontaminated by
any of the other concurrently-resolved same-named instances'
outcomes.every { it }
+
+ cleanup:
+ executor.shutdownNow()
}
void "concurrent resolution of the exact same shared ClassNode instance
from many threads is safe"() {
@@ -261,15 +268,17 @@ class AstPropertyResolveUtilsSpec extends Specification {
when: 'all threads race to resolve properties for the same instance at
once'
List<Future<List<String>>> futures = (0..<threadCount).collect {
executor.submit({ ->
- barrier.await()
+ barrier.await(30, TimeUnit.SECONDS)
AstPropertyResolveUtils.getPropertyNames(sharedNode)
} as Callable<List<String>>)
}
List<List<String>> results = futures.collect { Future<List<String>>
future -> future.get(30, TimeUnit.SECONDS) }
- executor.shutdown()
then: 'every thread observes the same, fully and correctly populated
result - none sees a partial or corrupted map'
results.every { it == ['label'] }
+
+ cleanup:
+ executor.shutdownNow()
}
private static Expression mapExpressionOf(String key, ClassNode valueType)
{
