jarredhj0214 commented on PR #12015:
URL: https://github.com/apache/gravitino/pull/12015#issuecomment-4976492565
> The `java.lang` mapping fixes the reported symptom, but the root cause is
broader: Janino probes name candidates one by one and relies on
`ClassNotFoundException` **without a cause** to mean "class doesn't exist, try
the next candidate" (see `ClassLoaderIClassLoader#findIClass`). Our
`loadClass()` wraps every failure into a CNFE **with a cause**, so any benign
probe miss is treated as a fatal load error.
>
> That's why this fix only covers names that happen to exist in `java.lang`:
e.g. generated code with `import java.util.*;` using a bare `List` still aborts
— the bare-name probe fails with a caused CNFE before Janino ever tries
`java.util.List`.
>
> Suggested change in `CustomURLClassLoader.loadClass()` — keep pure
not-found cause-less, wrap only real loading failures:
>
> ```java
> try {
> return clazz == null ? doLoadClass(name, resolve) : clazz;
> } catch (ClassNotFoundException e) {
> throw e; // benign miss: keep it cause-less for probe-driven callers
(Janino)
> } catch (Exception e) {
> throw new ClassNotFoundException("Failed to load " + name, e);
> }
> ```
>
> With this, the `java.lang` special case becomes unnecessary (Janino falls
back to `java.lang.Override` by itself via the shared `java.` path). If you
keep it as a compatibility shim, note it currently shadows any legitimate
default-package class whose name collides with a `java.lang` one (`Compiler`,
`Process`, `Record`, ...), since it runs before the normal lookup. A regression
test with `import java.util.*;` + bare simple name would lock in the general
fix.
Thanks for the detailed explanation. I updated the PR to follow this
direction.
Instead of resolving `java.lang` simple names in `IsolatedClassLoader`, the
latest change preserves the original `ClassNotFoundException` for normal class
probe misses and only wraps non-CNFE loading failures. This keeps Janino's
probing behavior intact, so it can continue trying candidate names such as
`java.lang.Override` or imported classes like `java.util.List`.
I also removed the `java.lang` simple-name shortcut to avoid shadowing
legitimate default-package classes, and added a regression test to verify that
probe misses are not wrapped with a cause.
--
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]
