daniellansun commented on code in PR #3109:
URL: https://github.com/apache/groovy/pull/3109#discussion_r4525453541
##########
src/main/java/groovy/lang/MetaClassRegistry.java:
##########
@@ -161,6 +169,41 @@ private MetaClass createWithCustomLookup(Class theClass,
MetaClassRegistry regis
}
}
+ private static boolean customMetaClassVisible(String customName) {
+ ClassLoader loader = MetaClassRegistry.class.getClassLoader();
+ if (loader == null) {
+ return true;
+ }
+ for (ClassLoader current = loader; current != null; current =
current.getParent()) {
+ if (current instanceof GroovyClassLoader
+ && ((GroovyClassLoader)
current).getClassCacheEntry(customName) != null) {
+ return true;
+ }
+ }
+ return classResourcePresent(loader, customName);
+ }
+
+ /**
+ * True when {@code className} is visible as a class file. A native
image
+ * with exact reachability metadata throws if that resource was not
+ * registered; the class may still be loadable, so the caller falls
+ * through to {@code Class.forName}.
+ */
+ private static boolean classResourcePresent(ClassLoader loader, String
className) {
+ try {
+ return loader.getResource(className.replace('.', '/') +
".class") != null;
+ } catch (Error error) {
+ if (isMissingResourceRegistration(error)) {
Review Comment:
`MetaClassRegistry.class.getClassLoader() == null` means this class was
defined by the bootstrap loader. There is no `ClassLoader` object to walk, and
`getResource` on null would throw. `Class.forName(String)` then uses that same
bootstrap loader (it is `Class.forName(name, true, caller.getClassLoader())`).
Returning false would skip that call and drop a custom MetaClass that really is
on the boot classpath. Returning true means "not shown to be absent": we fall
through to `Class.forName`, and a miss is still the `ClassNotFoundException`
path that calls `createNormalMetaClass`. The system loader is a different
loader, so probing it would not match what `Class.forName` does from this
class. The cost is that this one configuration does not avoid the
`parallelLockMap` entry; Groovy on the boot classpath is the case we cannot
probe.
##########
src/main/java/groovy/lang/MetaClassRegistry.java:
##########
@@ -143,7 +143,15 @@ public final MetaClass create(Class theClass,
MetaClassRegistry registry) {
private MetaClass createWithCustomLookup(Class theClass,
MetaClassRegistry registry) {
try {
- final Class customMetaClass =
Class.forName("groovy.runtime.metaclass." + theClass.getName() + "MetaClass");
+ final String customName = "groovy.runtime.metaclass." +
theClass.getName() + "MetaClass";
+ // Class.forName records a missing name in every
parallel-capable
+ // loader it asks, and the entry is never removed. The lookup
can
+ // only succeed for a class already defined by this runtime's
+ // loader or present as a classpath resource.
+ if (!customMetaClassVisible(customName)) {
Review Comment:
The second point is a real hole. The comment says the lookup can only
succeed for a class already defined by this runtime's loader or present as a
`.class` resource. That is stronger than what `Class.forName` does.
`customMetaClassVisible` returns false, and we never call `Class.forName`, when
no ancestor `GroovyClassLoader` has the name in `getClassCacheEntry` and
`getResource(binaryName + ".class")` returns null. A class loader can define a
class with no resource behind it: `defineClass(byte[])`, a `findClass` that
synthesizes bytes, or a class already defined in a non-`GroovyClassLoader`
parent (`findLoadedClass` succeeds inside `loadClass` with no `.class` URL).
Those are missed, and `createNormalMetaClass` is used instead. A native image
has the same shape when the class is registered for reflection and the resource
is not in the image. `getResource` then returns null. We already treat
`MissingResourceRegistrationError` as "still try `Class.forName`" because the
class may be
loadable without a registered resource; a null resource is that situation
without the exception, and it currently takes the opposite branch.
`GroovyClassLoader.hasClassResource` treats that error as a miss on purpose, so
the parent is not asked. This lookup should not copy that, because skipping
here changes which MetaClass the user gets.
The check is there because `Class.forName` on a parallel-capable loader
records every name in `parallelLockMap` from `getClassLoadingLock`, and that
entry is never removed. Each generated script otherwise leaves
`groovy.runtime.metaclass.<script>MetaClass` in the application loader and its
parents. A `.class` resource, a `GroovyClassLoader` cache hit, a null loader,
and the native-image registration error are reasons to call `Class.forName`. A
null resource is not a reason to skip it for an arbitrary class.
The skip should be limited to the names that grow without bound: the
`groovy.runtime.metaclass.<generated>MetaClass` companion of a script or
template class, where no parent can define that name and no `.class` resource
exists. A custom MetaClass that is a loaded class without a `.class` resource,
and any stable class whose companion a loader can still define, should still go
through `Class.forName`. `CustomMetaClassTest` only covers the classpath
`.class` case (`groovy.runtime.metaclass.bugs.CustomMetaClassTestMetaClass`),
so it does not catch this miss.
I will limit the skip to generated-script companion names that have no
resource and that no parent can define, and add a test for a custom MetaClass
defined without a `.class` resource.
--
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]
