paulk-asert commented on PR #2755:
URL: https://github.com/apache/groovy/pull/2755#issuecomment-5173819415
I haven't done a proper review yet, but as part of some other work, I
assessed whether the PR impacts potential GraalVM support if we try harder to
support that in the future. It came back with below, I'm not sure we want to do
what is says yet - but just wanted to capture it somewhere for now:
> Two native-image observations from exercising this branch alongside the
packed-closure work (GROOVY-12227). Both are small; the first is a genuine easy
win.
>
> ### 1. The soft-fail contract doesn't hold under native image
>
> `tryDefineNestmate` catches `IllegalAccessException | SecurityException |
LinkageError` (plus `IllegalArgumentException` and
`IndexOutOfBoundsException`), and the class javadoc documents the intent as
returning `null` on the expected failure modes "so call sites fall back to
`ClassLoader#defineClass` with one null check".
>
> GraalVM signals "this runtime cannot define classes" with
`com.oracle.svm.core.jdk.UnsupportedFeatureError`, which extends
`java.lang.Error` **directly**:
>
> ```
> $ javap com/oracle/svm/core/jdk/UnsupportedFeatureError.class # from
lib/svm/builder/svm.jar, GraalVM CE 25.2.4
> public class com.oracle.svm.core.jdk.UnsupportedFeatureError extends
java.lang.Error {
> ```
>
> It is not a `LinkageError`, so it escapes the catch and propagates out of
the `try*` method — in precisely the environment where the fallback matters
most.
>
> A blanket `catch (Throwable)` would contradict the javadoc's "unexpected
failures ... are not swallowed as a blanket `RuntimeException`", so something
targeted is probably wanted, e.g.:
>
> ```java
> } catch (Error e) {
> // GraalVM native image: runtime class definition is unsupported.
Name-checked
> // to avoid a build-time dependency on org.graalvm.
> if
("com.oracle.svm.core.jdk.UnsupportedFeatureError".equals(e.getClass().getName()))
{
> return null;
> }
> throw e;
> }
> ```
>
> ### 2. The kill switch is baked in at build time
>
> ```java
> public static final boolean HIDDEN_CLASSES_DISABLED =
> SystemUtil.getBooleanSafe(PROPERTY_DISABLE, false);
> ```
>
> `static final`, documented as "evaluated once at class-init so hot paths
pay no property-lookup cost". Under native image this class is very likely
initialized at *build* time, so the value captured is the build JVM's, and
`-Dgroovy.hidden.classes.disable=true` at run time silently does nothing — the
one escape hatch a native user would reach for.
>
> This is the same trap I hit in GROOVY-12227: I ended up evaluating the
equivalent check per link rather than caching it in a static, because a
build-time-initialized class bakes in the wrong answer (the image-code property
reports `buildtime` there, not `runtime`).
>
> ### Possibly one fix for both
>
> If `isEnabled()` did a per-call check that also returned `false` when
>
`"runtime".equals(System.getProperty("org.graalvm.nativeimage.imagecode"))`,
then the native path would never attempt the definition at all, the kill switch
would work at run time, and (1) becomes belt-and-braces rather than
load-bearing. If the hot-path cost of the property read is the concern, a
non-final holder initialised on first *use* rather than at class-init keeps
both properties.
>
> ### Caveat on incidence
>
> I have not observed (1) fire in practice — the coercion I tested (`[run: {
... }] as Runnable`) goes through `java.lang.reflect.Proxy` and works natively
on both master and this branch, so it does not reach `HiddenClassDefiner`. This
is from reading the code plus confirming the class hierarchy, not from a
reproduced failure. Worth a targeted test if you think the proxy/reflector
paths are reachable in a native image.
--
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]
