jdaugherty commented on code in PR #16034:
URL: https://github.com/apache/grails-core/pull/16034#discussion_r3652199198
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -94,22 +117,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);
+ Map<String, ClassNode> cachedProperties =
cachedClassProperties.get(classNode);
if (cachedProperties == null) {
- cachedProperties = new HashMap<>();
+ Map<String, ClassNode> newProperties = new HashMap<>();
boolean isDomainClass = AstUtils.isDomainClass(classNode);
if (isDomainClass) {
- cachedProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
- cachedProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
}
- cachedClassProperties.put(className, cachedProperties);
ClassNode currentNode = classNode;
while (currentNode != null &&
!currentNode.equals(ClassHelper.OBJECT_TYPE)) {
- populatePropertiesForClassNode(currentNode, cachedProperties,
isDomainClass, !isDomainClass);
+ populatePropertiesForClassNode(currentNode, newProperties,
isDomainClass, !isDomainClass);
currentNode = currentNode.getSuperClass();
}
- } return cachedProperties;
+ // Publish only once fully populated so a concurrent reader can
never observe a
+ // partially-populated entry for this ClassNode.
+ cachedProperties = newProperties;
+ cachedClassProperties.put(classNode, cachedProperties);
Review Comment:
The reorder-and-publish-last fix is correct, and `synchronizedMap` does
supply the safe publication it depends on: `get` and `put` synchronize on the
same mutex, so a reader that observes the entry also observes a fully-populated
`HashMap`. Worth stating that explicitly in the inline comment, because
"publish only once fully populated" is only sufficient *given* that
happens-before edge — with a bare `HashMap` the reordering alone wouldn't have
been enough.
One remaining wrinkle: the check-then-act across the `get` on line 120 and
the `put` on line 136 is not atomic, so two concurrent callers can each build a
complete map and the later `put` wins. Benign here (the maps are equivalent and
never mutated after publication), but
`cachedClassProperties.computeIfAbsent(classNode, ...)` on the synchronized
wrapper would be atomic and shorter — the tradeoff being that it holds the
mutex for the whole superclass walk.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a
static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same
source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated
sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately
share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name-
or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one
class with those of
+ * an unrelated class that happens to share its name. This spec proves the
cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never
contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
+
+ void "property lookups for two same-named ClassNodes in different packages
do not corrupt each other"() {
+ given: 'two distinct ClassNodes with the same simple name declared in
different packages'
+ ClassNode first = new ClassNode('org.example.one.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('org.example.two.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ when: 'the first class node is resolved, populating its cache entry'
+ List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
+
+ then: 'only its own property is resolved'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+
+ when: 'the second, differently-packaged, same-simple-name class node
is resolved'
+ List<String> secondProperties =
AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'its own property is resolved, not leaked from the first class
node'
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+
+ and: 'the first class node cache entry remains unaffected by resolving
the second'
+ List<String> firstPropertiesAfter =
AstPropertyResolveUtils.getPropertyNames(first)
+ firstPropertiesAfter.contains('color')
+ !firstPropertiesAfter.contains('weight')
+ }
+
+ void "property lookups for two distinct ClassNode instances with the exact
same unqualified name do not corrupt each other"() {
+ given: 'two distinct ClassNode instances - as produced by two separate
compilations - sharing an identical unqualified name'
+ ClassNode first = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ expect: 'the two ClassNode instances compare equal by name - the exact
condition that would collide in a name-keyed or equals()-keyed cache'
+ first == second
+ first.hashCode() == second.hashCode()
Review Comment:
These two assert Groovy's own `ClassNode` equality contract rather than
anything about `AstPropertyResolveUtils`. They hold today — 5.0.7's
`ClassNode.equals` compares `getText()` and `hashCode()` delegates to
`getText().hashCode()` — but if Groovy ever moves `ClassNode` to identity
equality this spec fails while the production behaviour it guards is still
perfectly correct.
`!first.is(second)` on line 79 is the precondition the test actually needs.
Consider keeping that and demoting the other two to a comment explaining *why*
two same-named nodes used to collide.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -47,7 +49,28 @@
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties
= new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link
ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()}
(essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode}
itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to
share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled
without a package (common
+ * in tests and dynamically generated sources), and for the same source
compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its
own {@code ClassNode}
+ * instance that must never share cached property data with another
compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision
entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST
transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel
test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for
concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code
put()} calls.
+ */
+ protected static final Map<ClassNode, Map<String, ClassNode>>
cachedClassProperties =
+ Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
Keying this **static** map by `ClassNode` identity does fix the collision,
but it converts a bounded cache into a classloader leak.
Under the old `String` key the map held at most one entry per distinct class
name. With `ClassNode` keys it holds one entry per *instance*, and a primary
`ClassNode` transitively pins its entire compilation:
```
ClassNode.getModule() -> ModuleNode.getUnit() -> CompileUnit.loader
(GroovyClassLoader)
```
plus `ClassNode.clazz` directly for resolved nodes. Since nothing ever
removes an entry and the field is now `static final`, every compilation that
runs a GORM AST transform inside a long-lived JVM permanently retains that
compilation's classloader and every class it loaded. That is not hypothetical:
the Gradle Groovy compiler daemon is reused across `compileGroovy` tasks and
across builds, and dev-mode / `GroovyClassLoader`-driven recompiles land in the
same place. The map values are `ClassNode`s too, so they pin loaders as well —
that part pre-existed, but it was O(distinct class names) and is now unbounded
in the number of compilations.
Groovy already stores per-node state exactly this way
(`ClassNode.getModule()` above is itself `getNodeMetaData(ModuleNode.class)`),
so the cleanest fix is to drop the static map and hang the cache off the node:
```java
private static final String PROPERTIES_CACHE_KEY =
AstPropertyResolveUtils.class.getName() + ".properties";
private static Map<String, ClassNode> getPropertiesFromCache(ClassNode
classNode) {
return classNode.getNodeMetaData(PROPERTIES_CACHE_KEY, cn ->
computeProperties(classNode));
}
```
`getNodeMetaData(Object, Function)` is available in Groovy 5, is
identity-scoped by construction (so the collision this PR fixes cannot arise at
all), needs no global lock, and is collected together with the node. Worth
deciding explicitly whether the holder should be `classNode` or
`classNode.redirect()` — `getModule()` uses `redirect()`, and redirected nodes
are the one case where the two differ.
If a process-wide map has to stay for some reason, it needs weak identity
keys and/or an explicit eviction point, plus a note documenting the expected
lifecycle.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -47,7 +49,28 @@
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties
= new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link
ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()}
(essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode}
itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to
share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled
without a package (common
+ * in tests and dynamically generated sources), and for the same source
compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its
own {@code ClassNode}
+ * instance that must never share cached property data with another
compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision
entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST
transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel
test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for
concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code
put()} calls.
Review Comment:
This rationale is inaccurate in a way that will mislead the next reader.
`maxParallelForks = configuredTestParallel` (`gradle/test-config.gradle:86`)
forks **separate JVMs**, and each JVM gets its own copy of a `static` field —
parallel test forks can therefore never race on this map. JUnit's in-JVM
parallel execution isn't enabled anywhere in the build either.
The concurrency exposure that does exist is the compiler itself: multiple
`compileGroovy` tasks running concurrently in a shared Gradle worker, and any
embedded compilation driven from more than one thread. Worth rewording to that,
otherwise the comment justifies the synchronization with a scenario that can't
happen.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a
static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same
source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated
sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately
share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name-
or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one
class with those of
+ * an unrelated class that happens to share its name. This spec proves the
cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never
contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
+
+ void "property lookups for two same-named ClassNodes in different packages
do not corrupt each other"() {
+ given: 'two distinct ClassNodes with the same simple name declared in
different packages'
+ ClassNode first = new ClassNode('org.example.one.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('org.example.two.Widget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ when: 'the first class node is resolved, populating its cache entry'
+ List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
+
+ then: 'only its own property is resolved'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+
+ when: 'the second, differently-packaged, same-simple-name class node
is resolved'
+ List<String> secondProperties =
AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'its own property is resolved, not leaked from the first class
node'
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+
+ and: 'the first class node cache entry remains unaffected by resolving
the second'
+ List<String> firstPropertiesAfter =
AstPropertyResolveUtils.getPropertyNames(first)
+ firstPropertiesAfter.contains('color')
+ !firstPropertiesAfter.contains('weight')
+ }
+
+ void "property lookups for two distinct ClassNode instances with the exact
same unqualified name do not corrupt each other"() {
+ given: 'two distinct ClassNode instances - as produced by two separate
compilations - sharing an identical unqualified name'
+ ClassNode first = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ first.addProperty('color', Modifier.PUBLIC, ClassHelper.STRING_TYPE,
null, null, null)
+
+ ClassNode second = new ClassNode('Widget', Modifier.PUBLIC,
ClassHelper.OBJECT_TYPE)
+ second.addProperty('weight', Modifier.PUBLIC,
ClassHelper.Integer_TYPE, null, null, null)
+
+ expect: 'the two ClassNode instances compare equal by name - the exact
condition that would collide in a name-keyed or equals()-keyed cache'
+ first == second
+ first.hashCode() == second.hashCode()
+ !first.is(second)
+
+ when: 'both class nodes are resolved'
+ List<String> firstProperties =
AstPropertyResolveUtils.getPropertyNames(first)
+ List<String> secondProperties =
AstPropertyResolveUtils.getPropertyNames(second)
+
+ then: 'each keeps its own, independently-resolved properties despite
comparing equal'
+ firstProperties.contains('color')
+ !firstProperties.contains('weight')
+ secondProperties.contains('weight')
+ !secondProperties.contains('color')
+ }
+
+ void "getPropertyType resolves and caches the type of a declared
property"() {
+ given: 'a class node with a declared property'
+ ClassNode classNode = new ClassNode('org.example.PropertyTypeWidget',
Modifier.PUBLIC, ClassHelper.OBJECT_TYPE)
+ classNode.addProperty('label', Modifier.PUBLIC,
ClassHelper.STRING_TYPE, null, null, null)
+
+ expect: 'the resolved property type matches the declared type, both on
first (cache-populating) and second (cache-hit) lookup'
+ AstPropertyResolveUtils.getPropertyType(classNode, 'label') ==
ClassHelper.STRING_TYPE
+ AstPropertyResolveUtils.getPropertyType(classNode, 'label') ==
ClassHelper.STRING_TYPE
Review Comment:
The name says "and caches", but neither assertion can distinguish a cache
hit from a miss — on a miss `getPropertyType` falls through to
`classNode.getProperty(propertyName)` and returns the same `STRING_TYPE`, so
both lines pass with caching entirely disabled.
To actually pin the caching behaviour: resolve once, then add a second
property to the `ClassNode` and assert `getPropertyNames` still returns the
*stale* view. That snapshot-at-first-lookup semantics is what this class really
guarantees and what the transform relies on, and it's currently unverified.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -47,7 +49,28 @@
* @since 6.1
*/
public class AstPropertyResolveUtils {
- protected static Map<String, Map<String, ClassNode>> cachedClassProperties
= new HashMap<>();
+
+ /**
+ * Cache of resolved properties per {@link ClassNode}.
+ * <p>
+ * Keyed by {@code ClassNode} identity rather than name. {@link
ClassNode#equals(Object)} and
+ * {@link ClassNode#hashCode()} compare by {@link ClassNode#getText()}
(essentially the class
+ * name), so a {@code Map} keyed by name - or even by {@code ClassNode}
itself as the map key -
+ * treats any two distinct {@code ClassNode} instances that happen to
share a name as the same
+ * cache entry. That collision is a real hazard for classes compiled
without a package (common
+ * in tests and dynamically generated sources), and for the same source
compiled more than once
+ * in separate {@code GroovyClassLoader}s: each compilation produces its
own {@code ClassNode}
+ * instance that must never share cached property data with another
compilation's instance of a
+ * same-named class. An {@link IdentityHashMap} avoids that collision
entirely by comparing keys
+ * with {@code ==} instead of {@code equals()}.
+ * <p>
+ * Wrapped in {@link Collections#synchronizedMap(Map)} because AST
transforms that populate and
+ * read this cache can run concurrently on multiple threads (e.g. parallel
test execution within
+ * one JVM/fork); a plain, unsynchronized {@link HashMap} is not safe for
concurrent structural
+ * modification and can corrupt its internal state under concurrent {@code
put()} calls.
+ */
+ protected static final Map<ClassNode, Map<String, ClassNode>>
cachedClassProperties =
Review Comment:
Two breaking changes to a `protected` member of a public class on one line:
the key type changed (`String` -> `ClassNode`) and the field became `final`.
Anything downstream that reassigned it — today the *only* way to clear this
cache, which plugin AST transforms and test harnesses plausibly do — now fails
to compile rather than merely behaving differently.
If it's being broken anyway, take it the rest of the way: make it `private
static final` and expose an explicit, documented `clearCache()`. That shrinks
the exposed surface and gives the retention problem above an escape hatch,
instead of leaving `protected` visibility on a field nobody can usefully touch
any more.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtilsSpec.groovy:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.datastore.gorm.transform
+
+import java.lang.reflect.Modifier
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import spock.lang.Specification
+
+/**
+ * {@link AstPropertyResolveUtils} caches resolved property metadata in a
static, process-wide
+ * map keyed by {@link ClassNode}. Two distinct compilations (e.g. the same
source parsed in two
+ * different {@code GroovyClassLoader}s, as happens for dynamically-generated
sources and in
+ * tests) produce distinct {@code ClassNode} instances that can legitimately
share the exact same
+ * name - {@code ClassNode#equals(Object)} compares by name, so a naive name-
or equals()-based
+ * cache key would conflate them, corrupting the resolved properties of one
class with those of
+ * an unrelated class that happens to share its name. This spec proves the
cache keys strictly by
+ * {@code ClassNode} identity, so same-named-but-distinct class nodes never
contaminate each
+ * other's cached property data.
+ */
+class AstPropertyResolveUtilsSpec extends Specification {
Review Comment:
Good to see this class get its first coverage. As written, though, it only
exercises hand-built non-domain `ClassNode`s, so the branches that actually
matter to the transform never execute: `AstUtils.isDomainClass` is false in all
three features, which means the injected `id`/`version` entries, the
`hasMany`/`belongsTo`/`hasOne` handling in
`populatePropertiesForInitialExpression`, the `isResolved()` /
`ClassPropertyFetcher` path, and the superclass walk are all untested. This PR
rewrites the population loop feeding every one of those, so per the repo rule
that a touched class gets its behaviour verified they should be covered here —
an `@Entity`-annotated `ClassNode` with a `hasMany` initial expression and a
domain superclass would reach most of it.
Also missing: a test for the concurrency fix the PR claims. Several threads
calling `getPropertyNames` concurrently on the same node and on distinct nodes,
asserting every returned list is complete, would exercise both the synchronized
map and the publish-after-populate ordering. As it stands, reverting either
half of the concurrency change leaves this spec green.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/transform/AstPropertyResolveUtils.java:
##########
@@ -94,22 +117,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);
+ Map<String, ClassNode> cachedProperties =
cachedClassProperties.get(classNode);
if (cachedProperties == null) {
- cachedProperties = new HashMap<>();
+ Map<String, ClassNode> newProperties = new HashMap<>();
boolean isDomainClass = AstUtils.isDomainClass(classNode);
if (isDomainClass) {
- cachedProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
- cachedProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.IDENTITY, new
ClassNode(Long.class));
+ newProperties.put(GormProperties.VERSION, new
ClassNode(Long.class));
}
- cachedClassProperties.put(className, cachedProperties);
ClassNode currentNode = classNode;
while (currentNode != null &&
!currentNode.equals(ClassHelper.OBJECT_TYPE)) {
- populatePropertiesForClassNode(currentNode, cachedProperties,
isDomainClass, !isDomainClass);
+ populatePropertiesForClassNode(currentNode, newProperties,
isDomainClass, !isDomainClass);
currentNode = currentNode.getSuperClass();
}
Review Comment:
Identity keying makes the cache immune to name collisions, but I don't think
it removes the nondeterminism that a flake like #16030 needs.
`populatePropertiesForClassNode` consults `ClassPropertyFetcher` only when
`classNode.isResolved()` (line 172), so an entry is a snapshot of whatever
resolution state the node happened to be in at the *first* lookup, and it is
never refreshed afterwards. If the first lookup for a node lands at a different
compilation phase between runs, the cached property set still differs between
runs — identity keys or not.
That also weakens the root-cause story in the description: compiling the
*same* source twice produces two `ClassNode`s with identical property sets, so
a name-keyed collision between just those two is harmless and can't produce
divergent bytecode on its own. The collisions that would actually diverge are
with a *differently-shaped* same-named class, or with a same-named node whose
entry was cached at a different resolution state.
Can you pin down which one you observed — e.g. the two colliding class
names, or the flake reproducing in a loop pre-fix and not post-fix? The change
is an improvement regardless, but if the resolution-state snapshot is the real
driver then #16030 comes back and this gets recorded as already fixed.
--
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]
