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

Unable to use sync cache with reactivestreams #31861

Closed
krizajb opened this issue Dec 18, 2023 · 14 comments
Closed

Unable to use sync cache with reactivestreams #31861

krizajb opened this issue Dec 18, 2023 · 14 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: feedback-provided Feedback has been provided type: regression A bug that is also a regression
Milestone

Comments

@krizajb
Copy link

krizajb commented Dec 18, 2023

Since commit f99faac that affected sync cache, I believe one can not integrate sync cache anymore sincespring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java enforces the use of Cache.retrieve when reactivestreams are present.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 18, 2023
@snicoll
Copy link
Member

snicoll commented Dec 18, 2023

@krizajb thanks for the report but I am not sure what you mean by "enforcing the use of Cache.retrieve". Can you please provide a bit more details as what was working and what is the problem now?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 18, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Dec 18, 2023

The asynchronous Cache.retrieve arrangement has support for a sync-like model as well as independent retrieve/put steps. We process CompletableFuture and reactive types specifically now, treating their contained values as cache values rather than the wrapper objects but we still adhere to sync=true vs sync=false semantics via delegating to the corresponding Cache.retrieve variant.

Also, in terms of Reactive Streams detection, we do check the method signature for well-known reactive types when Reactive Streams is present. However, if the method signature does not indicate a reactive method or a CompletableFuture result, we still perform regular Cache.get access as before. So the presence of the Reactive Streams API just leads to such introspection of the signature, not enforcing Cache.retrieve itself.

@krizajb
Copy link
Author

krizajb commented Dec 18, 2023

Providing a sample code

@EnableCaching
@Configuration
public class ServiceCacheConfifuation extends CachingConfigurerSupport {

  public static final String CACHE_NAME = "competition-cache";
  
  @Bean
  @Override
  public CacheManager cacheManager() {
    SimpleCacheManager cacheManager = new SimpleCacheManager();
    cacheManager.setCaches(List.of(new CaffeineCache(CACHE_NAME, Caffeine.newBuilder().build())));
    return cacheManager;
  }
}
@Service
public class ServiceProvider {

  @CachePut(value = CACHE_NAME)
  public Mono<Competition> getCompetitionDirect() {
    return createMonoWithCache();
  }

  @Cacheable(value = CACHE_NAME, sync = true)
  public Mono<Competition> getCompetition() {
    return createMonoWithCache();
  }
}

This will produce the following exception java.lang.IllegalStateException: No Caffeine AsyncCache available: set CaffeineCacheManager.setAsyncCacheMode(true). This can be fixed if async cache is provided in configuration, however I do not wish to do so.

This worked without a problem with springboot 3.1.6. Upgrading to 3.2.0 produces the mentioned issue. Thank you.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 18, 2023
@jhoeller
Copy link
Contributor

So this is not related to sync=true (as I assumed) but rather to your use of @Cacheable on a Mono-returning method in combination with a non-async target cache.

In general, Spring's new reactive caching support is meant to handle such Mono-returning methods automatically, caching the value that the Mono produces rather than the wrapper itself, and providing a new Mono for the cached value in case of a cache hit. This works with any Mono, no special preparation necessary.

So you are relying on a caching a custom Mono instance that you specifically prepared for being cached through Mono.cache() or the like? In general, we advise against that and would suggest caching the produced value of the original active Mono instead (which is what 6.1 does by default now).

That said, I can see your argument that this is a regression for custom scenarios, so we may consider some way of opting out of the async cache arrangement. We could do so automatically but on the other hand we do not want to lose the setup exception for reactive cache arrangements that are meant to use an async cache (just possibly having forgotten to configure it).

@krizajb
Copy link
Author

krizajb commented Dec 18, 2023

Oh, understood. What about the usage of @CachePut which always causes the method to be invoked and its result to be stored in the associated cache? Thanks!

@jhoeller
Copy link
Contributor

All of the caching annotations understand CompletableFuture/Mono/Flux now and store the produced values (or list of values) in the cache now, automatically re-adapting them for a cache hit. This works for @Cacheable as well as @CachePut and does not require any special preparations, just a regular CompletableFuture or Reactive Streams Publisher that we can attach to (e.g. waiting for the value to be requested and therefore produced and then automatically storing it in the cache).

@krizajb
Copy link
Author

krizajb commented Dec 19, 2023

I was not able to make it work with the additional info you provided. I tried removing the Mono.cache() calls and the cache configuration. Care for an example? Thank you.

@jhoeller
Copy link
Contributor

jhoeller commented Dec 19, 2023

With a CaffeineCacheManager and setAsyncCacheMode(true), it should work out of the box. Or as above, you could also do new CaffeineCache(CACHE_NAME, Caffeine.newBuilder().buildAsync(), true).

Any use of @Cacheable and @CachePut on a reactive method should automatically interact with the retrieve/put facilities on the configured Cache then, storing and retrieving the produced values.

@krizajb
Copy link
Author

krizajb commented Dec 19, 2023

This produces an async behaviour that isn't desired.

@jhoeller
Copy link
Contributor

jhoeller commented Dec 19, 2023

Well, the way we add those interactions to the reactive pipeline as a pre-step for a potential cache hit and a follow-up step to producing its value, the interactions have to be asynchronous. Regular Cache.get can block which must be avoided for the reactive pipeline. So for that new interaction model, this has to be a required setup, I'm afraid.

Theoretically you may keep using synchronous cache interactions with Mono.cache() if we add an escape hatch for this (which we could do for 6.1.3). That said, please be aware that you are adding potential blocking to your getCompetition() call then when Cache.get is trying to look for a cache hit.

Why do you prefer manually enforced caching through Mono.cache() over asynchronous cache interactions, actually? Or in other words, why is asynchronous behavior undesirable for an environment which is inherently asynchronous through the use of a Mono-based reactive pipeline already?

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Dec 19, 2023
@krizajb
Copy link
Author

krizajb commented Dec 20, 2023

It's an existing code that operates like that, since the framework supported it I guess.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 20, 2023
@snicoll
Copy link
Member

snicoll commented Dec 20, 2023

I don't think that answers our question. Here it is again

Or in other words, why is asynchronous behavior undesirable for an environment which is inherently asynchronous through the use of a Mono-based reactive pipeline already?

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Dec 20, 2023
@krizajb
Copy link
Author

krizajb commented Dec 20, 2023

Since the app depends on a certain sequence of events. To be exact, if @CachePut is called before @Cacheable, the instance freshly cached must be returned by the @Cacheable. This can not be guaranteed with the use of async cache.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 20, 2023
@jhoeller jhoeller added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 21, 2023
@jhoeller jhoeller self-assigned this Dec 21, 2023
@jhoeller jhoeller added this to the 6.1.3 milestone Dec 21, 2023
@jhoeller
Copy link
Contributor

On the timing concerns between cache put and lookup operations, with a local cache this won't be very different from a synchronous cache in practice. Even with retrieve, Caffeine is able to determine a cache hit immediately, just lazily retrieving the associated value. And put remains a synchronous hand-off operation even in asynchronous mode. As a consequence, I suppose that Caffeine's async cache mode specifically might work for your scenario as well, and we generally recommend a migration to caching the produced values in 6.1 style going forward.

That said, given this report and #31868, there seem to be several scenarios where such manual adaptation through Mono.cache()/Flux.cache() has worked with synchronous local caches before. While we recommend the 6.1 way of caching produced values, for scenarios where such a revision is not immediately possible/desirable, we introduce set the system property "spring.cache.reactivestreams.ignore=true" (can also be a similar entry in a spring.properties file on the classpath) as an escape hatch. I'll use this ticket for the introduction of that flag in 6.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: feedback-provided Feedback has been provided type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants