[
https://issues.apache.org/jira/browse/GROOVY-12303?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108902#comment-18108902
]
ASF GitHub Bot commented on GROOVY-12303:
-----------------------------------------
daniellansun commented on code in PR #2834:
URL: https://github.com/apache/groovy/pull/2834#discussion_r3873675506
##########
src/main/java/org/codehaus/groovy/control/ClassNodeResolver.java:
##########
@@ -304,28 +324,91 @@ private static LookupResult findByClassLoading(final
String name, final Compilat
return tryAsScript(name, compilationUnit, null);
} catch (CompilationFailedException cfe) {
throw new GroovyBugError("The lookup for " + name + " caused a
failed compilation. There should not have been any compilation from this
call.", cfe);
+ } catch (NoClassDefFoundError ncdfe) {
+ return recoverFromNoClassDefFoundError(name, compilationUnit,
loader, ncdfe);
}
- //TODO: The case of a NoClassDefFoundError needs a bit more research;
- // a simple recompilation is not possible it seems. The current class
- // we are searching for is there, so we should mark that somehow.
- // Basically the missing class needs to be completely compiled before
- // we can again search for the current name.
- /*catch (NoClassDefFoundError ncdfe) {
- cachedClasses.put(name,SCRIPT);
- return false;
- }*/
if (cls == null) return null;
- //NOTE: we might return false here even if we found a class,
- // because we want to give a possible script a chance to
- // recompile. This can only be done if the loader was not
- // the instance defining the class.
+ // NOTE: even if we found a class we still give a possible script a
chance
+ // to recompile, but only when this loader was not the instance that
defined it.
ClassNode cn = ClassHelper.make(cls);
if (cls.getClassLoader() != loader) {
return tryAsScript(name, compilationUnit, cn);
}
return new LookupResult(null,cn);
}
+ /**
+ * Recovers from {@link NoClassDefFoundError} thrown while defining or
linking
+ * {@code name}. The class being looked up is present — a referenced type
is not —
+ * so treating the failure as a miss would poison {@link #resolveName} with
+ * {@link #NO_CLASS} and hide bytecode or a groovy source that can still
be used.
+ * <p>
+ * Recovery order:
+ * <ol>
+ * <li>ASM decompilation of matching bytecode, which does not link the
class,
+ * so a missing superclass or interface does not prevent producing a
+ * {@link ClassNode}.</li>
+ * <li>If a {@code .class} resource exists for this path but declares a
+ * different binary name (the JVM {@code defineClass} name check,
also
+ * seen on case-insensitive filesystems), treat the lookup as a miss
and
+ * fall through to script lookup. Detection uses the bytecode name,
not
+ * {@link NoClassDefFoundError} text.</li>
+ * <li>A groovy source of the same name, added to the compilation
queue.</li>
+ * </ol>
+ * If none of those succeed the error is rethrown with {@code name} in the
+ * message so callers see both the class under lookup and the missing type.
+ */
+ private LookupResult recoverFromNoClassDefFoundError(final String name,
final CompilationUnit compilationUnit,
+ final GroovyClassLoader loader, final NoClassDefFoundError ncdfe) {
+ LookupResult decompiled = findDecompiled(name, compilationUnit,
loader, false);
Review Comment:
Thank you — that reading of the recovery path was right, and the patch has
been restructured around it.
### 1. The extra `findDecompiled` in recovery
When `asmResolving` is on we have already parsed the resource. Calling
`findDecompiled` again on the same name was redundant; a hit would have
returned before `loadClass`.
The guard you wondered about is in the caller (`tryAsLoaderClassOrScript`),
not in `findDecompiled`. So when `asmResolving` is off, that first parse never
ran — and that is the GROOVY-12303 case (unlinkable bytecode, class-loader
lookup only). There we still decompile **once**, as a last resort, because
`loadClass` cannot produce a `ClassNode` for a type that failed to link. That
hatch is no longer an unmarked second copy of the happy-path decompile.
`recoverFromNoClassDefFoundError` is gone. The first pass stores a
`ClassFile` (`MATCH` / `MISMATCH` / `ABSENT`, or `null` if ASM was not
attempted). The `NoClassDefFoundError` arm uses that result; it parses only
when the value is `null`.
### 2. `classFileDeclaresDifferentName` belongs on the first parse
Agreed. The bytecode-name check is now part of that single ASM read
(`readClassFile`). A mismatch is no longer collapsed to “not found” and
rediscovered with a second `DecompiledClassNode`.
The check still requires bytecode, so it does not run as a free-standing
decompile when ASM was already skipped — except in the last-resort hatch above,
which is the first parse of that lookup. We do not inspect
`NoClassDefFoundError` text (HotSpot’s `"wrong name"` phrase is not a portable
API).
A mismatch means this resource is not `name`. `loadClass` still runs
afterwards: `getResource` and `defineClass` / `findLoadedClass` are independent
(in-memory class of the right name, case-insensitive `getResource`). If
`loadClass` then throws `NoClassDefFoundError` and we already know the resource
was a mismatch, that is treated as “the requested name never existed”
(`tryAsScript` with no `oldClass`), not as a linkage failure of `name`.
### 3. `tryAsScript(name, cu, null)` is not legal for `NoClassDefFoundError`
Agreed. That was the CNFE contract (`oldClass == null` ⇒ any groovy source
wins, no `isSourceNewer`, no origin check). `NoClassDefFoundError` means some
loader found a class (or bytes it tried to derive). Replacing it with a script
is only legal when that class is from a parent — the same rule as the success
path (`cls.getClassLoader() != loader`) and as the decompile path
(`isFromAnotherClassLoader` then `tryAsScript(name, cu, decompiled)` so
`isSourceNewer` applies).
That is now the only class→script path. Same-loader unlinkable bytecode
keeps the `DecompiledClassNode`. If there is no matching class-file resource,
we cannot get a timestamp or a defining loader, so we rethrow the wrapped
`NoClassDefFoundError` and do not cache `NO_CLASS` — including when a groovy
source of the same name happens to exist.
Thank you again for catching this.
> ClassNodeResolver: NoClassDefFoundError during class-loader lookup aborts
> resolution
> ------------------------------------------------------------------------------------
>
> Key: GROOVY-12303
> URL: https://issues.apache.org/jira/browse/GROOVY-12303
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
>
> When class-loader lookup is used ({{{}asmResolving{}}} off, or the type
> exists only in memory), {{ClassNodeResolver}} calls
> {{{}GroovyClassLoader.loadClass{}}}. If the requested class *exists* but
> cannot be linked (missing superclass or interface), the JVM throws
> {{{}NoClassDefFoundError{}}}.
> That error used to escape resolution. Compilation aborted with an {{Error}}
> that named the {*}missing dependency{*}, not the type being resolved.
> {{resolveName}} could also cache the name as a miss ({{{}NO_CLASS{}}}), so a
> later successful compile of the dependency would not be retried.
> A TODO in {{findByClassLoading}} has noted this since the 2012 split out of
> {{{}ResolveVisitor{}}}.
> h3. Expected
> * If bytecode for the requested name is still on the class path, decompile
> it (ASM does not link) and continue.
> * Else if a groovy source of the same name is available, add it to the
> compilation queue.
> * Else if a {{.class}} resource exists for that path but declares a
> different binary name (JVM {{defineClass}} name check; also case-insensitive
> filesystems), treat the lookup as a miss. Detect this from the bytecode name,
> not from {{NoClassDefFoundError}} text (HotSpot's {{wrong name}} phrase is
> English-only; OpenJ9 does not use it).
> * Otherwise rethrow {{NoClassDefFoundError}} with the looked-up name in the
> message, and do not cache {{{}NO_CLASS{}}}.
> h3. Actual
> {{NoClassDefFoundError}} propagated out of
> {{{}ClassNodeResolver.findByClassLoading{}}}.
> h3. Reproducer
> Put {{HasDep.class}} (extends a type that is not loadable) on the class path,
> disable ASM resolving, and compile:
> {code:groovy}
> HasDep x = null
> {code}
> This fails with {{NoClassDefFoundError}} for the missing super-type instead
> of resolving {{{}HasDep{}}}.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
