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

Quarks Cache is not working in Bean that is created via a Producer #34436

Open
StefanMMS opened this issue Jun 30, 2023 · 27 comments
Open

Quarks Cache is not working in Bean that is created via a Producer #34436

StefanMMS opened this issue Jun 30, 2023 · 27 comments
Labels
area/cache kind/bug Something isn't working

Comments

@StefanMMS
Copy link

Describe the bug

I annotated a function with @CacheResult in a class that gets created as a bean by a producer.
The cache is created as I can see in the DEV UI, but no values are written to the cache.

If I create the bean by just adding the @ApplicationScoped annotation and remove the producer everything works fine.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

openjdk version "18.0.2" 2022-07-19

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.16.5.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Gradle 7.4

Additional information

No response

@StefanMMS StefanMMS added the kind/bug Something isn't working label Jun 30, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 30, 2023

/cc @gwenneg (cache)

@geoand
Copy link
Contributor

geoand commented Jun 30, 2023

We'd need a sample application the reproduces this behavior in order to investigate

@geoand geoand added the triage/needs-reproducer We are waiting for a reproducer. label Jun 30, 2023
@gsmet
Copy link
Member

gsmet commented Jul 3, 2023

This ^ and especially we need to see the @Produces method. Typically, are you also using the @ApplicationScoped scope for your @Produces method?

@StefanMMS
Copy link
Author

@Produces @ApplicationScoped @UnlessBuildProfile("prod") fun noOpFirebaseRemoteConfig(): RemoteConfigService = TestRemoteConfigService()

This is my producer for my local test environment.
I will create a test application to show this behavior.

@StefanMMS
Copy link
Author

TestApplication
https://github.com/sibeon/QuarkusCachingTest

@geoand
Copy link
Contributor

geoand commented Jul 6, 2023

I could not reproduce the problem with the sample application added above

@StefanMMS
Copy link
Author

This is a screenshot of the app when I am running it via InteliJ

image

@geoand
Copy link
Contributor

geoand commented Jul 7, 2023

Okay, I was able to reproduce it after all.

@gwenneg can you have a look at this one? It does seem important to fix this one

@geoand geoand removed the triage/needs-reproducer We are waiting for a reproducer. label Jul 7, 2023
@gwenneg
Copy link
Member

gwenneg commented Jul 7, 2023

I don't see any @Produces annotation in the reproducer. Is there a Kotlin equivalent that I'm not aware of?

@gwenneg
Copy link
Member

gwenneg commented Jul 7, 2023

CacheResultInterceptor is not invoked when the method annotated with @CacheResult is invoked.

@mkouba When:

  • a bean managed by CDI is instantiated from a method annotated with @Produces
  • that CDI bean contains a method annotated with an interceptor binding
  • the bean is injected and the method is invoked

Is the corresponding interceptor supposed to work?

@mkouba
Copy link
Contributor

mkouba commented Jul 10, 2023

Is the corresponding interceptor supposed to work?

No, it isn't. Interceptors and decorators are not applied to the return value of a producer method or the value of a producer field. (see Binding an interceptor to a bean).

And the reason is that the container does not control instantiation of the bean in that case.

In CDI 2.0 the jakarta.enterprise.inject.spi.InterceptionFactory got introduced but (A) it's not supported in Quarkus and (B) must be used directly in the producer method to create an intercepted instance.

@StefanMMS
Copy link
Author

I don't see any @Produces annotation in the reproducer. Is there a Kotlin equivalent that I'm not aware of?

https://quarkus.io/version/2.13/guides/cdi-reference#simplified-producer-method-declaration

@gwenneg
Copy link
Member

gwenneg commented Jul 12, 2023

So, @CacheResult or any other interceptor binding used on a method from a bean returned by a producer method cannot work because interceptors are simply not supported in Quarkus in that specific case.

@mkouba Is this mentioned in the CDI doc? I don't see it in https://quarkus.io/guides/cdi-reference#limitations but maybe it's somewhere else.

@mkouba
Copy link
Contributor

mkouba commented Jul 13, 2023

So, @CacheResult or any other interceptor binding used on a method from a bean returned by a producer method cannot work because interceptors are simply not supported in Quarkus in that specific case.

Not Quarkus, but any CDI implementation really.

@mkouba Is this mentioned in the CDI doc? I don't see it in https://quarkus.io/guides/cdi-reference#limitations but maybe it's somewhere else.

It's not a limitation of ArC. It's a "feature" of the CDI spec. The truth is that CDI Full has the InterceptionFactory which can by used by a producer method to overcome this limitation. But InterceptionFactory is not supported in Quarkus.

@geoand
Copy link
Contributor

geoand commented Jul 13, 2023

This default behavior of the spec seems very weird to me... Could we perhaps have some kind of Quarkus specific opt-in that would be utilized by cache (and whatever else we decide)?

@mkouba
Copy link
Contributor

mkouba commented Jul 13, 2023

This default behavior of the spec seems very weird to me...

It's not so weird when you think about the general purpose of producer methods - give the user the control of the instantiation of a bean, i.e. it's not the container that constructs the bean instance.

Also it's a bit challenging from technical point of view.

CC @Ladicek @manovotn

@geoand
Copy link
Contributor

geoand commented Jul 13, 2023

It's not so weird when you think about the general purpose of producer methods - give the user the control of the instantiation of a bean, i.e. it's not the container that constructs the bean instance.

From the implementors perspective, perhaps. But it's very surprising from a user's perspective (as is evident from this thread)

@Ladicek
Copy link
Contributor

Ladicek commented Jul 13, 2023

It is not "challenging", it is impossible. We have no idea what is the class of the object the producer method returns, so we can't create the subclass.

I mean, it is impossible in general, because the producer method may call other methods that create the ultimate object the producer returns. There are special cases we could support at the cost of the following heroics: perform control flow analysis on the bytecode of the method to determine if all code paths through the producer method end up instantiating and returning a known class, generate subclasses for those known classes, rewrite the bytecode of that producer so that those code paths return the subclasses instead. Pretty sure we don't want to do that.

@geoand
Copy link
Contributor

geoand commented Jul 13, 2023

In that case, can we at least log a strong worded warning?

@mkouba
Copy link
Contributor

mkouba commented Jul 13, 2023

But it's very surprising from a user's perspective (as is evident from this thread)

@geoand Yes, but it's not supported for good reasons. BTW the same applies to synthetic beans, because only managed beans (aka beans implemented by Java classes) support interception.

There are special cases we could support at the cost of the following heroics: ...

Or generate the subclass at runtime... which is not idiomatic for quarkus, nor an option for native image.

In that case, can we at least log a strong worded warning?

We could. Until someone else complains that those warnings are superfluous for this particular use case (it wouldn't be for the first time ;-).

@Ladicek
Copy link
Contributor

Ladicek commented Jul 13, 2023

I don't think we should add a warning for something that works exactly as it should. I mean, it is not just interceptors that don't work for producer beans, @Inject doesn't work, @PostConstruct and @PreDestroy don't work, probably a few other things I don't remember at the moment don't work either.

What we might be able to do is some build-time friendly variant of InterceptionFactory. @mkouba, do you think something like this might work?

@Produces
@RequestScoped
@SupportInterceptionOf(MyClass.class) // name subject to bike shedding of course
public MyClass produceMyClass(InterceptionFactory factory) {
    return factory.createInterceptedInstance(new MyClass());
}

That would mean:

  1. We'd pregenerate subclasses for the classes given in the @SupportInterceptionOf annotation.
  2. If the InterceptionFactory receives an object whose class is present in the list given by @SupportInterceptionOf, it would instantiate the subclass with the given object as a delegate. So it wouldn't be like our current subclasses, where we delegate to the superclass, we'd have to delegate to another object, but I believe it's the exact same with regular InterceptionFactory.
  3. If the InterceptionFactory receives an object whose class is not present in the list given by @SupportInterceptionOf, it would throw an exception.

Alternatively, we could ditch CDI's InterceptionFactory and use something of our own that would work similarly to managed beans (that is, delegation to superclass instead of a second instance). Something like:

@Produces
@RequestScoped
@SupportInterceptionOf(MyClass.class) // name subject to bike shedding of course
public MyClass produceMyClass(ArcInterceptedBeanFactory factory) { // name intentionally ugly
     // could also accept arguments to pass to the constructor, probably?
    return factory.createInterceptedInstance(MyClass.class);
}

@Ladicek
Copy link
Contributor

Ladicek commented Jul 13, 2023

(Looking at the InterceptionFactory API again, we wouldn't be able to support configure() and ignoreFinalMethods() anyway -- createInterceptedInstance() is the only thing we could possibly do. So creating our own API is probably a good idea regardless of the "interception style".)

@mkouba
Copy link
Contributor

mkouba commented Jul 13, 2023

@Ladicek pls add your observations to the existing issue: #3532 ;-)

@Ladicek
Copy link
Contributor

Ladicek commented Jul 13, 2023

Ah, I didn't know we have an issue for that. Will copy my comments there.

@gsmet
Copy link
Member

gsmet commented Jul 13, 2023

I’m not sure I would add a warning in the logs but we need a warning in the documentation. And I think we should also point to this warning in the cache doc.

@manovotn
Copy link
Contributor

In that case, can we at least log a strong worded warning?

Don't think it's detectable that easily. Technically, your class Foo with interceptor bindings can, at the same time, be a return value of a producer as well as a class bean on its own - in one case you'd want to log it, in the other you wouldn't.
Mentioning this in docs sounds fine.

@manovotn
Copy link
Contributor

If I create the bean by just adding the @ApplicationScoped annotation and remove the producer everything works fine.

BTW, if you can do this, you should.
Class-based beans should be a go-to choice for bean declaration that also supports the interception you are looking for (as well as injection into bean and so on).
Producers are a way to handle beans whose whole lifecycle cannot be fully handled by CDI or when you need to meet some special criteria such as returning different beans or even null based on certain condition. The extra versatility it provides in bean creation comes with extra drawbacks too, as you found out the hard way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cache kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants