Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache: fix CacheInterceptor in case of non-ArC interceptor bindings #43832

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Oct 11, 2024

RESTEasy Classic's MP RestClient implementation produces annotations at runtime, so they are not created by ArC and therefore don't extend AbstractAnnotationLiteral. At the same time, that implementation produces an ArcInvocationContext and puts interceptor bindings into its context map under the ArC key.

Some places may expect that an ArcInvocationContext would always contain ArC-created AbstractAnnotationLiteral instances, but alas, per the description above, that is not the case.

There are multiple options for fixing that collision. My preferred one would be to get rid of AbstractAnnotationLiteral and treat all annotations uniformly. That unfortunately has negative performance implications on the CacheInterceptor, so is not an option yet [1].

This commit chooses another path: it modifies the only place in Quarkus that actually depends on AbstractAnnotationLiteral to check whether the Set<AbstractAnnotationLiteral> actually contains instances of AbstractAnnotationLiteral. I hope that before more places in Quarkus start depending on AbstractAnnotationLiteral, we can get rid of it.

This commit only checks the first annotation in the set, because if the bindings come from RESTEasy Classic, then none of them are instances of AbstractAnnotationLiteral, and if they come from ArC, then all of them are instances of AbstractAnnotationLiteral.

[1] The performance issue (JDK-8180450) is fixed in JDK 23 and has
not been backported to any LTS release as of this writing.

Fixes #36336

RESTEasy Classic's MP RestClient implementation produces annotations
at runtime, so they are not created by ArC and therefore don't extend
`AbstractAnnotationLiteral`. At the same time, that implementation
produces an `ArcInvocationContext` and puts interceptor bindings
into its context map under the ArC key.

Some places may expect that an `ArcInvocationContext` would always
contain ArC-created `AbstractAnnotationLiteral` instances, but alas,
per the description above, that is not the case.

There are multiple options for fixing that collision. My preferred one
would be to get rid of `AbstractAnnotationLiteral` and treat all
annotations uniformly. That unfortunately has negative performance
implications on the `CacheInterceptor`, so is not an option yet [1].

This commit chooses another path: it modifies the only place in Quarkus
that actually depends on `AbstractAnnotationLiteral` to check whether
the `Set<AbstractAnnotationLiteral>` actually contains instances of
`AbstractAnnotationLiteral`. I hope that before more places in Quarkus
start depending on `AbstractAnnotationLiteral`, we can get rid of it.

This commit only checks the first annotation in the set, because
if the bindings come from RESTEasy Classic, then none of them are
instances of `AbstractAnnotationLiteral`, and if they come from ArC,
then all of them are instances of `AbstractAnnotationLiteral`.

[1] The performance issue (JDK-8180450) is fixed in JDK 23 and has
    not been backported to any LTS release as of this writing.
@mkouba
Copy link
Contributor

mkouba commented Oct 11, 2024

For the record, it worked in Quarkus 3.3.3 because there was no io.quarkus.restclient.runtime.QuarkusInvocationContextImpl and legacy rest-client invocation context did not implement the ArcInvocationContext. Therefore InterceptorBindings#getInterceptorBindingLiterals() returned null and CacheInterceptor#getNonArcCacheInterceptionContext() was used as a fallback. QuarkusInvocationContextImpl was introduced in #36123 to address problems like #36118.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 11, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6a4fbd6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/infinispan-cache/deployment

io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

  • expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@CacheResult on legacy @RegisterRestClient throws CCE on 3.4.2
3 participants