jdaugherty commented on code in PR #16034:
URL: https://github.com/apache/grails-core/pull/16034#discussion_r3793875034
##########
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:
Minor: both of these are redirect-less holder nodes, so under the new scheme
each becomes its own cache holder and never shares with anything. Line 236
already uses the plain-reference convention —
`ClassHelper.make(Long.class).getPlainNodeReference()` here would redirect to
the interned `Long` node so id/version lookups share a single entry again.
##########
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:
Of everything here, this is the change I would most like reverted. Given the
`@Memoized` `isDomainClass` collision (see the comment on `computeProperties`),
uniqueness *is* still required for correctness here — the original warning was
right.
The risk is concrete inside this module. `DirtyCheckTransformationSpec`
compiles `@DirtyCheck class Book` and `class Author` from strings, which are
not domain classes, while `QueryStringTransformerSpec` compiles `@Entity class
Book` and `class Author`, which are. `forkEvery = 50` on CI
(`gradle/test-config.gradle:88`) means up to fifty spec classes share one JVM,
so both can land in the same fork in either order, and whichever runs first
wins the memoized `isDomainClass` answer for that name.
A maintainer who takes this comment at face value and renames
`ClosureCaptureBook` back to `Book` walks straight into an order-dependent
flake of exactly the #16030 character.
Please keep the "must be unique" wording as a requirement. If you want to
record the history, the accurate version is that the *property map* is now
per-instance while the domain-class verdict is still name-keyed.
##########
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:
`AstUtils.isDomainClass(ClassNode)` is `@Memoized` (`AstUtils.groovy:428`),
and that memo cache is keyed by `ClassNode` *equality*, not identity —
`ClassNode.equals`/`hashCode` both reduce to `getText()`
(`ClassNode.java:1283`/`1288` in 5.0.7). `@Memoized` also defaults to
`maxCacheSize = 0`, so it is an unbounded `UnlimitedConcurrentCache` that never
evicts.
That leaves both headline claims in the new javadoc false one level down:
two distinct same-named `ClassNode`s do still collide here, and the first node
seen per distinct name is retained for the life of the JVM, pinning its module,
compile unit and `GroovyClassLoader` with it.
Verified against this branch with two hand-built nodes both named
`probe.MemoWidget`, the first plain and the second genuinely carrying
`@Entity`, resolved in that order:
```
plainProps = [title]
entityProps = [title] <- @Entity node, no id/version
isDomainClass(entity) = false <- despite the annotation
```
Control, the same `@Entity` node under a name nothing else had touched:
`[id, title, version]`.
To be fair to the change: the `@Memoized` predates this PR and you are not
introducing it. But this PR does newly route through it. Under the old
name-keyed map, a same-named second node returned the first node's cached map
and never called `isDomainClass` at all; now it computes its own properties and
consults the memoized verdict. The corruption narrows from the entire property
map to the domain-ness boolean, which is a genuine improvement — "no collision
is possible" just overstates where it lands.
I am not asking you to fix `isDomainClass` here; that deserves its own issue
rather than growing this PR. Just asking that the javadoc claims match what is
actually true.
##########
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:
This precondition never runs as an assertion. It sits in an `and:` block
continuing `given:`, and Spock applies implicit conditions only in
`then:`/`expect:` blocks, so the expression is evaluated and discarded.
Confirmed on this branch by putting `false` and `x == 999` in an `and:`
after `given:` in a scratch spec — the feature passed.
Needs `assert !first.is(second)`, or to move into an `expect:` block. Worth
correcting because the comment directly above presents this line as the
load-bearing premise of the feature.
##########
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 point as the equivalent comment in `WhereQueryClosureCaptureSpec`:
"that collision can no longer happen regardless of naming" does not hold while
`AstUtils.isDomainClass` is `@Memoized` by name, so the uniqueness requirement
should stay stated as a requirement rather than a stylistic preference.
##########
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:
`executor.shutdown()` is the last statement of the `when:` block, so it is
skipped on exactly the path where it matters: if any `future.get(30, SECONDS)`
throws, the `collect` aborts and the pool is never shut down.
`Executors.newFixedThreadPool` threads are non-daemon and do not time out, and
with `forkEvery = 50` this JVM goes on to run more specs.
`barrier.await()` also has no timeout, so if one worker dies before reaching
the barrier the rest park indefinitely and the only symptom is a 30-second
timeout per future.
Suggest a `cleanup:` block calling `executor.shutdownNow()` in both
concurrency features, plus the timeout overload of `await`. The
`GroovyClassLoader` in the reflection feature is also never closed — it is
`AutoCloseable`, and closing it would suit a spec whose subject is a
classloader leak.
##########
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:
`synchronized (cacheHolder)` cannot deliver the safety the javadoc claims
for it, because the nodes being locked are not always private to one
compilation. `QueryStringTransformer.groovy:208` walks a property path by
feeding each resolved type back in as the next receiver, so a JDK-typed
property makes an interned `ClassHelper` singleton the cache holder.
Verified on this branch:
```
propType.is(ClassHelper.STRING_TYPE) = true
STRING_TYPE cache metadata before = false
STRING_TYPE cache metadata after = true
```
`ClassNode.getModule()` reads that same `ListHashMap` through
`getNodeMetaData(ModuleNode.class)` without taking the node monitor, so this
lock only excludes callers that opt into it. The javadoc frames the leftover
exposure as risk that "belongs to `ClassNode`'s metadata storage in general",
but before this change the utility never wrote to any `ClassNode` — it is this
PR that makes GORM a writer to those JVM-wide maps.
Gating on `cacheHolder.isPrimaryClassNode()` and skipping the cache
otherwise would confine writes to nodes the compilation actually owns, and the
JDK types that get skipped are cheap to recompute.
Separately, `computeProperties` runs entirely inside this monitor, and for a
resolved domain class it reaches
`ClassPropertyFetcher.forClass(getTypeClass()).getPropertyValue(...)`, which
forces the user class's static initialiser and invokes user-written static
getters while the lock is held. Computing outside the lock and publishing
afterwards would take alien code out from under it.
--
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]
