You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@grails.apache.org by bo...@apache.org on 2026/07/21 21:52:34 UTC
(grails-core) branch fix/flaky-where-query-closure-capture-cache created (now 620f651fed)
This is an automated email from the ASF dual-hosted git repository.
borinquenkid pushed a change to branch fix/flaky-where-query-closure-capture-cache
in repository https://gitbox.apache.org/repos/asf/grails-core.git
at 620f651fed Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity
This branch includes the following new commits:
new 620f651fed Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(grails-core) 01/01: Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity
Posted by bo...@apache.org.
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
commit 620f651fedb9763d02309a60a5c2771338773794
Author: Walter Duque de Estrada <wb...@mac.com>
AuthorDate: Tue Jul 21 16:07:53 2026 -0500
Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity
WhereQueryClosureCaptureSpec ("generated closure constructors are identical
across compilations" and "association criteria closures capture only the
variables they reference") was ~2% flaky in CI with no hard failures on the
same commit - a classic sign of shared, racy static state rather than a test
bug.
Root cause: AstPropertyResolveUtils.cachedClassProperties is a static,
process-wide java.util.HashMap that the where{} query transform
(DetachedCriteriaTransformer) consults to resolve a domain class's property
names/types. Two problems compounded:
1. Not thread-safe. Gradle runs many spec classes concurrently within one
JVM/fork, so concurrent put()/resize on a plain HashMap can corrupt its
internal structure - a well known source of nondeterministic, rare
failures that reproduce inconsistently between reruns.
2. Keyed by name (ClassNode#getName()), and ClassNode#equals()/hashCode()
themselves compare by name too. Two distinct ClassNode instances that
share a name - e.g. the same source compiled twice into separate
GroovyClassLoaders, as WhereQueryClosureCaptureSpec's "identical across
compilations" test does on purpose, or any two test fixtures compiled
without a package - collide on the same cache entry. Whichever
compilation populates the entry first "wins", so the second compilation's
property resolution (and therefore the generated closure's captured
variables) can silently depend on stale data from an unrelated
ClassNode/classloader.
Fix: back the cache with an IdentityHashMap (keys compared by `==`, not
`equals()`), wrapped in Collections.synchronizedMap for thread safety. This
eliminates both the corruption hazard and the name-collision hazard
outright, since two different ClassNode instances can never share an entry
regardless of what they're named. Also stopped publishing the new entry into
the shared map until it is fully populated, so a concurrent reader can never
observe a partially-built entry.
Note: the cache already keyed by ClassNode#getName() (the fully-qualified
name), not a bare simple name as initially suspected - but for classes
compiled without a package (common in tests and generated sources), the
qualified name *is* the simple name, so name-based keying could not have
fixed the collision this spec's own "compile the same source twice" test
depends on. Identity-based keying removes the ambiguity entirely instead of
narrowing it.
Added AstPropertyResolveUtilsSpec, a new unit test that builds two distinct
ClassNode instances with the same unqualified name and different property
sets, and proves each resolves and caches its own properties independent of
the other - directly reproducing and proving the fix for the collision,
without relying on reruns of the flaky spec to catch it.
Verified: :grails-datamapping-core:test passes in full, including
WhereQueryClosureCaptureSpec and the new AstPropertyResolveUtilsSpec.
codeStyle (Checkstyle + CodeNarc) reports no violations.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
.../gorm/transform/AstPropertyResolveUtils.java | 44 +++++++--
.../transform/AstPropertyResolveUtilsSpec.groovy | 101 +++++++++++++++++++++
2 files changed, 136 insertions(+), 9 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 4deeed44ea..3d4495a4b1 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
@@ -20,7 +20,9 @@
package org.grails.datastore.gorm.transform;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
+import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
@@ -47,7 +49,28 @@ import org.grails.datastore.mapping.reflect.NameUtils;
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties = new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()} (essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode} itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled without a package (common
+ * in tests and dynamically generated sources), and for the same source compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its own {@code ClassNode}
+ * instance that must never share cached property data with another compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code put()} calls.
+ */
+ protected static final Map<ClassNode, Map<String, ClassNode>> cachedClassProperties =
+ Collections.synchronizedMap(new IdentityHashMap<>());
/**
* Resolves the type of of the given property
@@ -94,22 +117,25 @@ public class AstPropertyResolveUtils {
}
private static Map<String, ClassNode> getPropertiesFromCache(ClassNode classNode) {
- String className = classNode.getName();
- Map<String, ClassNode> cachedProperties = cachedClassProperties.get(className);
+ Map<String, ClassNode> cachedProperties = cachedClassProperties.get(classNode);
if (cachedProperties == null) {
- cachedProperties = new HashMap<>();
+ Map<String, ClassNode> newProperties = new HashMap<>();
boolean isDomainClass = AstUtils.isDomainClass(classNode);
if (isDomainClass) {
- cachedProperties.put(GormProperties.IDENTITY, new ClassNode(Long.class));
- cachedProperties.put(GormProperties.VERSION, new ClassNode(Long.class));
+ newProperties.put(GormProperties.IDENTITY, new ClassNode(Long.class));
+ newProperties.put(GormProperties.VERSION, new ClassNode(Long.class));
}
- cachedClassProperties.put(className, cachedProperties);
ClassNode currentNode = classNode;
while (currentNode != null && !currentNode.equals(ClassHelper.OBJECT_TYPE)) {
- populatePropertiesForClassNode(currentNode, cachedProperties, isDomainClass, !isDomainClass);
+ populatePropertiesForClassNode(currentNode, newProperties, isDomainClass, !isDomainClass);
currentNode = currentNode.getSuperClass();
}
- } return cachedProperties;
+ // Publish only once fully populated so a concurrent reader can never observe a
+ // partially-populated entry for this ClassNode.
+ cachedProperties = newProperties;
+ cachedClassProperties.put(classNode, cachedProperties);
+ }
+ return cachedProperties;
}
private static void populatePropertiesForClassNode(ClassNode classNode, Map<String, ClassNode> cachedProperties, boolean isDomainClass, boolean allowAbstract) {
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
new file mode 100644
index 0000000000..9bf4ab4f5d
--- /dev/null
+++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name- or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one class with those of
+ * an unrelated class that happens to share its name. This spec proves the cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
+
+ void "property lookups for two same-named ClassNodes in different packages do not corrupt each other"() {
+ given: 'two distinct ClassNodes with the same simple name declared in different packages'
+ ClassNode first = new ClassNode('org.example.one.Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null)
+
+ ClassNode second = new ClassNode('org.example.two.Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null)
+
+ when: 'the first class node is resolved, populating its cache entry'
+ List<String> firstProperties = AstPropertyResolveUtils.getPropertyNames(first)
+
+ then: 'only its own property is resolved'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+
+ when: 'the second, differently-packaged, same-simple-name class node is resolved'
+ List<String> secondProperties = AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'its own property is resolved, not leaked from the first class node'
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+
+ and: 'the first class node cache entry remains unaffected by resolving the second'
+ List<String> firstPropertiesAfter = AstPropertyResolveUtils.getPropertyNames(first)
+ firstPropertiesAfter.contains('color')
+ !firstPropertiesAfter.contains('weight')
+ }
+
+ void "property lookups for two distinct ClassNode instances with the exact same unqualified name do not corrupt each other"() {
+ given: 'two distinct ClassNode instances - as produced by two separate compilations - sharing an identical unqualified name'
+ ClassNode first = new ClassNode('Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null)
+
+ ClassNode second = new ClassNode('Widget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null)
+
+ expect: 'the two ClassNode instances compare equal by name - the exact condition that would collide in a name-keyed or equals()-keyed cache'
+ first == second
+ first.hashCode() == second.hashCode()
+ !first.is(second)
+
+ when: 'both class nodes are resolved'
+ List<String> firstProperties = AstPropertyResolveUtils.getPropertyNames(first)
+ List<String> secondProperties = AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'each keeps its own, independently-resolved properties despite comparing equal'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+ }
+
+ void "getPropertyType resolves and caches the type of a declared property"() {
+ given: 'a class node with a declared property'
+ ClassNode classNode = new ClassNode('org.example.PropertyTypeWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ classNode.addProperty('label', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null)
+
+ expect: 'the resolved property type matches the declared type, both on first (cache-populating) and second (cache-hit) lookup'
+ AstPropertyResolveUtils.getPropertyType(classNode, 'label') == ClassHelper.STRING_TYPE
+ AstPropertyResolveUtils.getPropertyType(classNode, 'label') == ClassHelper.STRING_TYPE
+ }
+}