borinquenkid commented on code in PR #16034:
URL: https://github.com/apache/grails-core/pull/16034#discussion_r3799323021
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -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);
Review Comment:
Agreed, and fixed in 0d195f1493. Corrected the javadoc's "no collision is
possible" claim to scope it explicitly to the resolved property map, and added
a paragraph documenting the `isDomainClass` memo-cache exposure you found —
noting it's a separate, pre-existing issue this class neither causes nor fixes.
Also restored the "must stay unique" wording in both `WhereQuery*Spec` comment
blocks accordingly (see replies below).
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -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) {
Review Comment:
Both points fixed in 0d195f1493.
`getPropertiesFromCache` now checks `classNode.isPrimaryClassNode()` first
and skips the cache entirely for non-primary nodes — which is exactly what
`ClassHelper.make(Class)`'s interned singletons are (verified against the 5.0.7
source: the `ClassNode(Class)` constructor sets `isPrimaryNode = false`). So
this class no longer writes its metadata key into a node it doesn't own,
closing the `QueryStringTransformer` path you traced. These nodes are
recomputed on every call instead, which is cheap given there are only a handful
of them.
Separately, `computeProperties` now runs outside the `synchronized
(cacheHolder)` block — the lock is only taken to check for and then publish the
result, so the static-initializer/getter execution via `ClassPropertyFetcher`
no longer happens while holding the per-node monitor. Two threads can both
compute for the same as-yet-uncached node; the second to acquire the lock
discards its own (equivalent) result and returns the one already published.
Updated the javadoc to describe both.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -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));
Review Comment:
Done — `IDENTITY`/`VERSION` now use
`ClassHelper.make(Long.class).getPlainNodeReference()`, matching the convention
already used for the `hasMany`/`belongsTo`/`hasOne` values a few lines down.
##########
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 -
Review Comment:
You're right, reverted. Restored the "must stay unique" wording and spelled
out why: the property-map fix doesn't cover `isDomainClass`'s own memoization,
so a same-named, differently-shaped fixture in another spec sharing this JVM
fork (`forkEvery = 50`) could still hand this class a stale domain-class
verdict from that memo cache. Renaming back to `Book`/`Author` would risk
exactly the order-dependent flake you described.
##########
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 -
Review Comment:
Same fix applied here — restored the uniqueness requirement with the same
rationale as the equivalent comment in `WhereQueryClosureCaptureSpec`.
##########
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<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)
+
+ 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)
Review Comment:
Confirmed and fixed — good catch, that precondition was silently never
checked. Added an explicit `assert`.
##########
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<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)
+
+ 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<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 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<String> 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<String> 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<String> 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<Future<Boolean>> futures = (0..<threadCount).collect { int i ->
+ 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<String> names =
AstPropertyResolveUtils.getPropertyNames(node)
+ names.contains(propertyName) && names.count {
it.startsWith('prop') } == 1
+ } as Callable<Boolean>)
+ }
+ List<Boolean> outcomes = futures.collect { Future<Boolean> future ->
future.get(30, TimeUnit.SECONDS) }
+ executor.shutdown()
Review Comment:
Fixed both concurrency tests: `executor.shutdownNow()` moved into a
`cleanup:` block so it runs even if a `future.get()` throws, and
`barrier.await()` now takes the 30s timeout so a dead worker fails fast instead
of parking the rest indefinitely. Also closed the `GroovyClassLoader` in the
reflection test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
