Fix flaky WhereQueryClosureCaptureSpec: key AstPropertyResolveUtils cache by ClassNode identity

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java

@@ -47,7 +47,77 @@
  * @since 6.1
  */
 public class AstPropertyResolveUtils {
-    protected static Map<String, Map<String, ClassNode>> cachedClassProperties = new HashMap<>();
+
+    /**
+     * Key under which the resolved property map is stashed via {@link ClassNode#getNodeMetaData(Object, java.util.function.Function)}.
+     * <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
+     * problem: a static map is never emptied, so every {@code ClassNode} ever looked up - and, for
+     * a primary node, the {@code GroovyClassLoader}/{@code CompileUnit} it pins via
+     * {@link ClassNode#getModule()} - is retained for the lifetime of the JVM. In a long-lived
+     * process that repeatedly compiles Groovy (a Gradle daemon reusing its Groovy compiler across
+     * builds, a dev-mode recompile loop), that is an unbounded classloader leak.
+     * <p>
+     * Storing the resolved properties as metadata on the {@code ClassNode} itself instead avoids
+     * both hazards this class has previously had to fix:
+     * <ul>
+     *     <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>
+     *     <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>
+     * 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
+     * shares one cache entry. {@code redirect()} can in principle be reassigned after construction
+     * (e.g. a forward-reference placeholder later pointed at the real, resolved node by the
+     * compiler's resolve phase) - but {@link ClassNode#setRedirect(ClassNode)} explicitly refuses to
+     * do so for a <em>primary</em> node ({@code GroovyBugError}), and every {@code ClassNode} this
+     * utility's real callers pass in - one actively being compiled from source during an AST
+     * transform - is primary, so for them {@code redirect()} is simply {@code this} for the entire
+     * object's life; there is no reassignment to account for. The one place a non-primary,
+     * redirect-able node does appear ({@link ClassHelper#make(Class, boolean) ClassHelper.make(c,
+     * false)}) sets its redirect target eagerly, at construction, before any caller could observe or
+     * cache through it in an unredirected state. Even so, if some future, currently-unforeseen path
+     * ever did cache through a node before it was redirected, that would be harmless rather than a
+     * source of stale data: the entry becomes unreachable the moment the redirect is set (a later
+     * lookup through the same original reference resolves to the different, real node and computes
+     * fresh there instead), so it is simply never read again and becomes eligible for garbage
+     * collection with the abandoned node.
+     * <p>
+     * {@code classNode.isResolved()} gates one part of what gets cached (see
+     * {@link #populatePropertiesForClassNode}) and, in principle, its result can depend on when it
+     * is checked - it delegates through {@code redirect} (see {@link ClassNode#isResolved()}). Given
+     * the above, that delegation is immaterial for the primary nodes this cache actually serves: a
+     * terminal node's {@code isResolved()} reduces to {@code clazz != null} (or, for array/component
+     * types, {@code componentType.isResolved()}) - and {@code clazz} has no setter anywhere in
+     * {@code ClassNode} outside its {@code ClassNode(Class)} constructor (verified against the
+     * Groovy 5.0.7 sources), so that value is fixed for the object's entire life.
+     * <p>
+     * Access is synchronized per {@code ClassNode} ({@code synchronized (cacheHolder)} in
+     * {@link #getPropertiesFromCache}) because {@link ClassNode}'s node-metadata storage
+     * ({@code NodeMetaDataHandler}, backed by {@code ListHashMap}) is explicitly documented as not
+     * thread-safe, and while a given {@code ClassNode} representing a class actively being compiled
+     * is normally touched by only the one thread compiling it, that is not true of every node this
+     * utility can be called with: some callers resolve a property's declared type as a plain
+     * {@code ClassNode} for a common JDK type (e.g. an {@code Object}- or {@code def}-typed
+     * property), and {@link ClassHelper#make(Class)} returns a small, fixed set of interned,
+     * JVM-wide-shared singleton nodes for exactly those types (e.g. {@link ClassHelper#OBJECT_TYPE},
+     * {@code STRING_TYPE}). Two unrelated, concurrently-running compilations that both happen to
+     * resolve such a type would otherwise race on the same node's metadata map with no protection at
+     * all. Synchronizing per node makes concurrent calls into this class safe with respect to each
+     * other and keeps the common case (one thread, one node) effectively uncontended. It cannot, by
+     * itself, protect against unrelated code elsewhere in the compiler writing a <em>different</em>
+     * 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.
+     */
+    private static final String PROPERTIES_CACHE_KEY = AstPropertyResolveUtils.class.getName() + ".properties";
 
     /**
      * Resolves the type of of the given property
@@ -94,22 +164,25 @@ public static List<String> getPropertyNames(ClassNode classNode) {
     }
 
     private static Map<String, ClassNode> getPropertiesFromCache(ClassNode classNode) {
-        String className = classNode.getName();
-        Map<String, ClassNode> cachedProperties = cachedClassProperties.get(className);
-        if (cachedProperties == null) {
-            cachedProperties = 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));
-            }
-            cachedClassProperties.put(className, cachedProperties);
-            ClassNode currentNode = classNode;
-            while (currentNode != null && !currentNode.equals(ClassHelper.OBJECT_TYPE)) {
-                populatePropertiesForClassNode(currentNode, cachedProperties, isDomainClass, !isDomainClass);
-                currentNode = currentNode.getSuperClass();
-            }
-        } return cachedProperties;
+        ClassNode cacheHolder = classNode.redirect();
+        synchronized (cacheHolder) {
+            return cacheHolder.getNodeMetaData(PROPERTIES_CACHE_KEY, key -> computeProperties(cacheHolder));
+        }
+    }
+
+    private static Map<String, ClassNode> computeProperties(ClassNode classNode) {
+        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));
+        }
+        ClassNode currentNode = classNode;
+        while (currentNode != null && !currentNode.equals(ClassHelper.OBJECT_TYPE)) {
+            populatePropertiesForClassNode(currentNode, newProperties, isDomainClass, !isDomainClass);
+            currentNode = currentNode.getSuperClass();
+        }
+        return newProperties;
     }
 
     private static void populatePropertiesForClassNode(ClassNode classNode, Map<String, ClassNode> cachedProperties, boolean isDomainClass, boolean allowAbstract) {

grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryClosureCaptureSpec.groovy

@@ -31,8 +31,14 @@ import spock.lang.Specification
  */
 class WhereQueryClosureCaptureSpec extends Specification {
 
-    // The domain class names must be unique across the test JVM because
-    // AstPropertyResolveUtils caches resolved properties statically by class name
+    // 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.
     private static final String SERVICE_SOURCE = '''
 import grails.gorm.DetachedCriteria
 import grails.gorm.annotation.Entity

grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/WhereQueryEmbeddedBlockTransformSpec.groovy

@@ -33,8 +33,13 @@ import spock.lang.Specification
  */
 class WhereQueryEmbeddedBlockTransformSpec extends Specification {
 
-    // The domain class names must be unique across the test JVM because
-    // AstPropertyResolveUtils caches resolved properties statically by class name
+    // 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.
     private static final String SERVICE_SOURCE = '''
 import grails.gorm.DetachedCriteria
 import grails.gorm.annotation.Entity