Extend reflection configuration for SmallRye FT

[zakkak]

Doesn't seem to fix any functionality issues but prevents
MissingRegistrationErrors from being thrown when
ThrowMissingRegistrationErrors is enabled.

Related to #41995

[Ladicek]

LGTM for the fault tolerance change. Someone else should approve the Vert.x changes, although they seem like a no-brainer.

[gsmet]

CI looks unhappy:

2024-07-30T20:52:39.8530794Z Caused by: org.graalvm.nativeimage.MissingReflectionRegistrationError: The program tried to reflectively invoke method public java.lang.String io.quarkus.it.rest.client.main.FaultToleranceClient$$CDIWrapper.fallback() without it being registered for runtime reflection. Add public java.lang.String io.quarkus.it.rest.client.main.FaultToleranceClient$$CDIWrapper.fallback() to the reflection metadata to solve this problem. See https://www.graalvm.org/latest/reference-manual/native-image/metadata/#reflection for help.
2024-07-30T20:52:39.8536648Z 	at org.graalvm.nativeimage.builder/com.oracle.svm.core.reflect.MissingReflectionRegistrationUtils.forQueriedOnlyExecutable(MissingReflectionRegistrationUtils.java:72)
2024-07-30T20:52:39.8538942Z 	at java.base@21.0.4/java.lang.reflect.Method.acquireMethodAccessor(Method.java:77)
2024-07-30T20:52:39.8540282Z 	at java.base@21.0.4/java.lang.reflect.Method.invoke(Method.java:577)
2024-07-30T20:52:39.8542250Z 	at io.smallrye.faulttolerance.core.invocation.NormalMethodInvoker.proceed(NormalMethodInvoker.java:46)
2024-07-30T20:52:39.8544470Z 	at io.smallrye.faulttolerance.FaultToleranceInterceptor.lambda$prepareFallbackFunction$13(FaultToleranceInterceptor.java:575)
2024-07-30T20:52:39.8546131Z 	... 27 more

[Ladicek]

OK, that's weird. Since this PR only adds more reflection config to fault tolerance, that suggests there's more reflection configuration for the same class and they are not combined properly 🤷

[zakkak]

CI looks unhappy:

2024-07-30T20:52:39.8530794Z Caused by: org.graalvm.nativeimage.MissingReflectionRegistrationError: The program tried to reflectively invoke method public java.lang.String io.quarkus.it.rest.client.main.FaultToleranceClient$$CDIWrapper.fallback() without it being registered for runtime reflection. Add public java.lang.String io.quarkus.it.rest.client.main.FaultToleranceClient$$CDIWrapper.fallback() to the reflection metadata to solve this problem. See https://www.graalvm.org/latest/reference-manual/native-image/metadata/#reflection for help.
2024-07-30T20:52:39.8536648Z 	at org.graalvm.nativeimage.builder/com.oracle.svm.core.reflect.MissingReflectionRegistrationUtils.forQueriedOnlyExecutable(MissingReflectionRegistrationUtils.java:72)
2024-07-30T20:52:39.8538942Z 	at java.base@21.0.4/java.lang.reflect.Method.acquireMethodAccessor(Method.java:77)
2024-07-30T20:52:39.8540282Z 	at java.base@21.0.4/java.lang.reflect.Method.invoke(Method.java:577)
2024-07-30T20:52:39.8542250Z 	at io.smallrye.faulttolerance.core.invocation.NormalMethodInvoker.proceed(NormalMethodInvoker.java:46)
2024-07-30T20:52:39.8544470Z 	at io.smallrye.faulttolerance.FaultToleranceInterceptor.lambda$prepareFallbackFunction$13(FaultToleranceInterceptor.java:575)
2024-07-30T20:52:39.8546131Z 	... 27 more

io.quarkus.it.rest.client.main.FaultToleranceClient$$CDIWrapper is a generated class that looks like this:

@ApplicationScoped
@RestClient
@Typed({FaultToleranceClient.class})
@Path("/unprocessable")
public class FaultToleranceClient$$CDIWrapper extends RestClientReactiveCDIWrapperBase implements FaultToleranceClient {
   public FaultToleranceClient$$CDIWrapper() {
      ClassLoader var1 = Thread.currentThread().getContextClassLoader();
      Class var2 = Class.forName("io.quarkus.it.rest.client.main.FaultToleranceClient", false, var1);
      super(var2, "", "w-fault-tolerance", false);
   }

   @Consumes({"text/plain"})
   @Produces({"text/plain"})
   public String hello() {
      return ((FaultToleranceClient)this.getDelegate()).hello();
   }

   public String fallback() {
      return super.fallback();
   }

   @Fallback(
      fallbackMethod = "fallback"
   )
   public String helloWithFallback() {
      return super.helloWithFallback();
   }
}

Since helloWithFallback is annotated with @Fallback, the fallback method (i.e. the value of the annotation) should be registered for reflection (see

quarkus/extensions/smallrye-fault-tolerance/deployment/src/main/java/io/quarkus/smallrye/faulttolerance/deployment/SmallRyeFaultToleranceProcessor.java

Lines 124 to 159 in c9edbe6

	for (AnnotationInstance annotation : index.getAnnotations(DotNames.FALLBACK)) {
	AnnotationValue fallbackMethodValue = annotation.value("fallbackMethod");
	if (fallbackMethodValue == null) {
	continue;
	}
	String fallbackMethod = fallbackMethodValue.asString();
	Queue<DotName> classesToScan = new ArrayDeque<>(); // work queue
	// @Fallback can only be present on methods, so this is just future-proofing
	AnnotationTarget target = annotation.target();
	if (target.kind() == Kind.METHOD) {
	classesToScan.add(target.asMethod().declaringClass().name());
	}
	while (!classesToScan.isEmpty()) {
	DotName name = classesToScan.poll();
	ClassInfo clazz = index.getClassByName(name);
	if (clazz == null) {
	continue;
	}
	// we could further restrict the set of registered methods based on matching parameter types,
	// but that's relatively complex and SmallRye Fault Tolerance has to do it anyway
	clazz.methods()
	.stream()
	.filter(it -> fallbackMethod.equals(it.name()))
	.forEach(it -> reflectiveMethod.produce(new ReflectiveMethodBuildItem(it)));
	DotName superClass = clazz.superName();
	if (superClass != null && !DotNames.OBJECT.equals(superClass)) {
	classesToScan.add(superClass);
	}
	classesToScan.addAll(clazz.interfaceNames());
	}
	}

) but this is not happening.

I suspect/guess that the reason it's not happening is because the generated class is not indexed when processing the annotated methods.

@Ladicek @gsmet any clues how I can overcome this and get the method registered?

[zakkak]

OK, that's weird. Since this PR only adds more reflection config to fault tolerance, that suggests there's more reflection configuration for the same class and they are not combined properly 🤷

The added reflection seems to change the smallrye behavior though, since io.smallrye.faulttolerance.internal.SecurityActions.findDeclaredMethodNames used to fail with an exception return no methods before, but now it doesn't.

I wonder if this is a common pattern in places where the recorder is used, i.e. the recorder ensures things will be properly set up and work, while the library code silently fails when trying to configure things but without breaking things thanks to the recorder (similarly to #42140 (comment)).

[mkouba]

The Vert.x changes LGTM.

[zakkak]

This is now ready for review.

Some comments:

1. It looks like there are multiple places were Quarkus gets away without the "proper" reflection configuration. As stated in Extend reflection configuration for SmallRye FT #42229 (comment) I wonder if the approach I am taking is the right one, i.e. registering the missing stuff, or whether we should substitute the code performing these reflective accesses as Quarkus seems to work OK without them. I believe we will need some domain expert to decide on this on a per case bases. @Ladicek can you please have a look on this PR and provide some feedback regarding the above as far as FT is concerned? Thanks

2. It is based on Fix and refactor ReflectiveClassBuildItem #42265 (so please review only the commits specific to this PR)

Issues have been addressed.

[gsmet]

@Ladicek if you are still happy with the latest changes, I let you merge this one, thanks!

[Sanne]

Would it be possible to only register things for reflection as last resort?

I feel we should always try, in order:

1. patch the library to not need reflection
2. patch the extension to try resolve the reflection needs at build time, e.g. "run init during build" (and avoid runtime reflection)
3. register for reflection conditionally: only register the cases which are being used by the actual application instance (and couldn't be avoided in other ways)

[Ladicek]

I'm not sure this is done, actually. I'd prefer to wait for @zakkak to return from PTO before doing anything with this PR.

[zakkak]

After discussing this with @Ladicek we concluded that:

1. Enable --exact-reachability-metadata with Mandrel >= 23.1 (GraalVM for JDK 21) #41995 is a "nice to have" at the moment, but might become a "must have" in the future. The issues I am trying to resolve with this PR are due to the --exact-reachability-metadata flag, so they are not "actual" issues for the time being.
2. It is possible to do the work currently requiring reflection at build time, but hasn't been done yet due to not being trivial.
3. At some point it might even be possible to completely avoid using reflection, by moving to CDI method invokers and getting rid of reflection registration altogether, but @Ladicek is afraid we can't do that yet due to non-Quarkus usage
4. It looks like putting the effort to do the work at build time would result in "better" native code, but there is no hard evidence it is required at the moment. The increased footprint due to the reflection usage in this case is not that significant (if we add --exact-reachability-metadata to the mix then things get more interesting though).

With the above in mind I propose I move the VertX changes in a different PR and keep this PR as draft with just the smallrye-FT additions for future reference, hoping that eventually it might not be needed.

Adding the metadata for all methods to be able to be queried doesn't affect correctness in this case, but just avoids the exception thrown when using --exact-reachability-metadata.

[Ladicek]

With the above in mind I propose I move the VertX changes in a different PR and keep this PR as draft with just the smallrye-FT additions for future reference

Agreed.