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 4deeed44ea1..3ea291deb6e 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 @@ -47,7 +47,77 @@ * @since 6.1 */ public class AstPropertyResolveUtils { - protected static Map> cachedClassProperties = new HashMap<>(); + + /** + * Key under which the resolved property map is stashed via {@link ClassNode#getNodeMetaData(Object, java.util.function.Function)}. + *

+ * 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. + *

+ * Storing the resolved properties as metadata on the {@code ClassNode} itself instead avoids + * both hazards this class has previously had to fix: + *

+ * 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 primary 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. + *

+ * {@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. + *

+ * 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 different + * 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 getPropertyNames(ClassNode classNode) { } private static Map getPropertiesFromCache(ClassNode classNode) { - String className = classNode.getName(); - Map 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 computeProperties(ClassNode classNode) { + Map 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 cachedProperties, boolean isDomainClass, boolean allowAbstract) { 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 0870b3b240d..31ee016851d 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,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 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 9a516d651ad..8b2660ad9d9 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,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 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 00000000000..4ddc7a62040 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy @@ -0,0 +1,280 @@ +/* + * 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 java.util.concurrent.Callable +import java.util.concurrent.CyclicBarrier +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.Future +import java.util.concurrent.TimeUnit + +import org.codehaus.groovy.ast.AnnotationNode +import org.codehaus.groovy.ast.ClassHelper +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MapExpression +import spock.lang.Specification + +import grails.gorm.annotation.Entity +import org.grails.datastore.mapping.model.config.GormProperties + +/** + * {@link AstPropertyResolveUtils} caches resolved property metadata as metadata on the + * {@link ClassNode} it describes (see that class's javadoc). 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 is scoped strictly per {@code ClassNode} instance, so same-named-but-distinct + * class nodes never contaminate each other's cached property data, that domain-class-specific + * resolution (identity/version injection, association metadata) works, and that concurrent + * resolution of distinct nodes is safe. + */ +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 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 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 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) + + and: 'they are genuinely different instances - the precondition a name- or equals()-keyed cache would get wrong' + // ClassNode#equals()/hashCode() compare by getText() (essentially the class name), so + // first == second and first.hashCode() == second.hashCode() both hold here even though + // 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) + + when: 'both class nodes are resolved' + List firstProperties = AstPropertyResolveUtils.getPropertyNames(first) + List 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 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 and second lookup' + AstPropertyResolveUtils.getPropertyType(classNode, 'label') == ClassHelper.STRING_TYPE + AstPropertyResolveUtils.getPropertyType(classNode, 'label') == ClassHelper.STRING_TYPE + } + + void "getPropertyNames returns the snapshot taken on first lookup rather than reflecting properties added afterwards"() { + given: 'a class node with one declared property' + ClassNode classNode = new ClassNode('org.example.CachedSnapshotWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + classNode.addProperty('label', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + + when: 'the property names are resolved once, populating the cache' + List firstLookup = AstPropertyResolveUtils.getPropertyNames(classNode) + + then: + firstLookup == ['label'] + + when: 'a second property is added directly to the ClassNode after the cache has already been populated' + classNode.addProperty('extra', Modifier.PUBLIC, ClassHelper.Integer_TYPE, null, null, null) + + then: 'a direct lookup on the ClassNode confirms the property really was added - so the cache below is stale, not simply broken' + classNode.getProperty('extra') != null + + and: 'getPropertyNames still returns the cached snapshot from the first lookup, proving the result was actually cached rather than recomputed on every call' + !AstPropertyResolveUtils.getPropertyNames(classNode).contains('extra') + } + + void "getPropertyNames injects identity and version for a domain class and resolves hasMany/belongsTo/hasOne declared via AST initial expressions"() { + given: 'a domain class node declaring hasMany/belongsTo/hasOne as property initial expressions, as "static hasMany = [...]" compiles to' + ClassNode associatedType = new ClassNode('org.example.AssociatedThing', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + ClassNode classNode = new ClassNode('org.example.AstDrivenDomain', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + classNode.addAnnotation(new AnnotationNode(ClassHelper.make(Entity))) + classNode.addProperty('title', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + classNode.addProperty(GormProperties.HAS_MANY, Modifier.PUBLIC | Modifier.STATIC, ClassHelper.MAP_TYPE.getPlainNodeReference(), + mapExpressionOf('things', associatedType), null, null) + + when: + List propertyNames = AstPropertyResolveUtils.getPropertyNames(classNode) + + then: 'the AST-declared property is present alongside the injected identity/version properties' + propertyNames.containsAll(['title', GormProperties.IDENTITY, GormProperties.VERSION, 'things']) + AstPropertyResolveUtils.getPropertyType(classNode, GormProperties.IDENTITY) == new ClassNode(Long.class) + AstPropertyResolveUtils.getPropertyType(classNode, 'things') == associatedType + + and: 'the raw hasMany/belongsTo/hasOne map property itself is not exposed as a plain property' + !propertyNames.contains(GormProperties.HAS_MANY) + } + + void "getPropertyNames resolves hasMany/belongsTo/hasOne association metadata via reflection once the domain class is fully resolved"() { + given: 'a real, already-compiled domain class with hasMany/belongsTo/hasOne associations' + GroovyClassLoader gcl = new GroovyClassLoader() + gcl.parseClass(''' + import grails.gorm.annotation.Entity + + @Entity + class ReflectedAssociationAuthor { + String name + } + + @Entity + class ReflectedAssociationBook { + String title + } + + @Entity + class ReflectedAssociationPublisher { + String company + } + + @Entity + class ReflectedAssociationFixture { + static hasMany = [books: ReflectedAssociationBook] + static belongsTo = [author: ReflectedAssociationAuthor] + static hasOne = [publisher: ReflectedAssociationPublisher] + } + ''') + Class domainClass = gcl.loadedClasses.find { it.simpleName == 'ReflectedAssociationFixture' } + + and: 'a fresh ClassNode built from the already-compiled class, as happens once compilation has finished' + ClassNode resolvedNode = ClassHelper.make(domainClass) + + expect: 'the node reports itself resolved, which is what gates the reflection-based association lookup' + resolvedNode.isResolved() + + when: + List propertyNames = AstPropertyResolveUtils.getPropertyNames(resolvedNode) + + then: 'the reflected association properties are present alongside the injected identity/version properties' + propertyNames.containsAll([GormProperties.IDENTITY, GormProperties.VERSION, 'books', 'author', 'publisher']) + } + + void "concurrent resolution of distinct, identically-named ClassNode instances never corrupts each other's cached properties"() { + given: 'many threads, each building and resolving its own distinct ClassNode sharing one common name' + int threadCount = 20 + ExecutorService executor = Executors.newFixedThreadPool(threadCount) + CyclicBarrier barrier = new CyclicBarrier(threadCount) + + when: 'all threads race to populate the cache for their own instance at the same time' + List> futures = (0.. + executor.submit({ -> + barrier.await() + 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) + + List names = AstPropertyResolveUtils.getPropertyNames(node) + names.contains(propertyName) && names.count { it.startsWith('prop') } == 1 + } as Callable) + } + List outcomes = futures.collect { Future 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 } + } + + void "concurrent resolution of the exact same shared ClassNode instance from many threads is safe"() { + // Distinct-instance concurrency (the test above) can never exercise a race on the + // underlying node-metadata storage, because nothing is shared between the threads. A single + // ClassNode instance genuinely can be looked up from more than one thread at once in + // practice - e.g. ClassHelper.OBJECT_TYPE/STRING_TYPE are JVM-wide singletons that this + // utility's callers can resolve to for a plain Object- or def-typed property, so two + // unrelated, concurrently-running compilations could both reach this cache for the exact + // same node. This test exercises that shared-node case directly and asserts every thread's + // returned value is correct. + // + // Note on what this test can and can't prove: the cached computation here is deterministic + // and idempotent, so even with the "synchronized (cacheHolder)" guard in + // AstPropertyResolveUtils#getPropertiesFromCache removed, every thread still computes and + // returns the same correct value in practice - a black-box test of returned values cannot + // reliably force ClassNode's underlying, explicitly-documented-not-thread-safe metadata + // storage into an observably-wrong state without reaching into Groovy internals neither this + // spec nor AstPropertyResolveUtils controls. Verified by temporarily removing that guard and + // running this test 8 times without a failure. The synchronization is kept as a correctness + // fix justified by ListHashMap's own documentation, not because this test can demonstrate + // its absence breaking anything; this test instead guards against a regression to something + // observably broken (an exception, a null, a wrong/partial result) under real concurrent + // load, which is the failure mode a future refactor could plausibly introduce. + given: 'one ClassNode instance that every thread will resolve concurrently' + ClassNode sharedNode = new ClassNode('SharedWidget', Modifier.PUBLIC, ClassHelper.OBJECT_TYPE) + sharedNode.addProperty('label', Modifier.PUBLIC, ClassHelper.STRING_TYPE, null, null, null) + int threadCount = 32 + ExecutorService executor = Executors.newFixedThreadPool(threadCount) + CyclicBarrier barrier = new CyclicBarrier(threadCount) + + when: 'all threads race to resolve properties for the same instance at once' + List>> futures = (0.. + barrier.await() + AstPropertyResolveUtils.getPropertyNames(sharedNode) + } as Callable>) + } + List> results = futures.collect { Future> 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'] } + } + + private static Expression mapExpressionOf(String key, ClassNode valueType) { + MapExpression mapExpression = new MapExpression() + mapExpression.addMapEntryExpression(new ConstantExpression(key), new ClassExpression(valueType)) + return mapExpression + } +}