Skip to content

The LocalResponseCacheGatewayFilterFactory is creating a new CacheManager for each Filter #3025

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

Closed
michael-wirth opened this issue Aug 11, 2023 · 15 comments
Labels
Milestone

Comments

@michael-wirth
Copy link

michael-wirth commented Aug 11, 2023

Describe the bug
The LocalResponseCacheGatewayFilterFactory is not using the "gatewayCacheManger" bean, but always creating a new instance.
Is this a bug or correct by design?

Cache routeCache = LocalResponseCacheAutoConfiguration.createGatewayCacheManager(cacheProperties)

@ilozano2
Copy link

That's intentional. You have two types of Cache for LocalResponseCache

  • Global: if you activate spring.cloud.gateway.global-filter.local-response-cache.enabled all the responses will be stored in the same cache unless you use LocalResponseCache filter at the route level
  • At Route level: if you add the filter LocalResponseCache in a route, it will use its own independent cache with its own configuration

@michael-wirth
Copy link
Author

I see the point of having an independent cache for each route, but why is it needed to instantiate a new cache-manager for each route?

If there are e.g. 5 routes with LocalResponseCache fiter then it will result in 5 cache-managers each having 1 cache. With that solution it is impossible to customize the cache manager.

@alimodares2003
Copy link

alimodares2003 commented Sep 16, 2023

See here:
https://github.com/spring-cloud/spring-cloud-gateway/blob/eb098a759634f5e4898eb0963b83b966ae1b0063/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/config/LocalResponseCacheAutoConfiguration.java#L49C8-L49C8

I think because proxyBeanMethods is set to false, each time creates a new cache-manager that its Bean is defined in line 71 of the LocalResponseCacheAutoConfiguration class

@michael-wirth
Copy link
Author

I doubt that setting proxyBeanMethods to true would change anything because LocalResponseCacheAutoConfiguration.createGatewayCacheManager(cacheProperties) is not a @Bean method

@alimodares2003
Copy link

alimodares2003 commented Sep 17, 2023

But it called in Bean method:

@Bean(name = RESPONSE_CACHE_MANAGER_NAME)
@Conditional(LocalResponseCacheAutoConfiguration.OnGlobalLocalResponseCacheCondition.class)
public CacheManager gatewayCacheManager(LocalResponseCacheProperties cacheProperties) {
	return createGatewayCacheManager(cacheProperties);
}

@michael-wirth
Copy link
Author

michael-wirth commented Sep 17, 2023

That is what I'm saying.

The gatewayCacheManager Bean is NOT used in the LocalResponseCacheGatewayFilterFactory!
Please check the code at

Cache routeCache = LocalResponseCacheAutoConfiguration.createGatewayCacheManager(cacheProperties)

The cacheManager should be reused instead of creating a new instance for each route.

@alimodares2003
Copy link

Yes you're right, It's like anti-pattern!

@mohhmekk
Copy link

mohhmekk commented Nov 12, 2023

Same question from my side, why would you create a separate cache manager for each route.
In my case, I need to create and use my own cache manager, according to the documentation I can by annotating my cache manager as primary but it is never being used.

A workaround that i have done is wrapping the filter inside another component and inject my cache manager myself:

@Component
class ResponseCacheFilterWrapper @Autowired constructor(responseCacheManager: ResponseCacheManager) : GlobalFilter, Ordered {
    val responseFilter: ResponseCacheGatewayFilter = ResponseCacheGatewayFilter(responseCacheManager)
    val log: Log = LogFactory.getLog(javaClass)

    override fun filter(exchange: ServerWebExchange?, chain: GatewayFilterChain?): Mono<Void> {
        log.info("Caching Response filter is active")
        return responseFilter.filter(exchange, chain)
    }

    override fun getOrder(): Int =
        responseFilter.order
}

The cache configuration looks like the following:

class CacheConfig {
    @Bean
    fun caffeineConfig(): Caffeine<Any, Any>? =
         Caffeine.newBuilder().expireAfterWrite(30, TimeUnit.MINUTES)

    @Bean
    @Primary
    fun cacheManager(caffeine: Caffeine<Any, Any>): CacheManager {
        val caffeineCacheManager = CaffeineCacheManager()
        caffeineCacheManager.setCaffeine(caffeine)
        return caffeineCacheManager
    }

    @Bean
    fun cacheName():String = "ALL-CACHES"

    @Bean
    fun responseCacheManager(cacheManager: CacheManager, cacheName: String): ResponseCacheManager =
        ResponseCacheManager(CacheKeyGenerator(), cacheManager.getCache(cacheName), Duration.ofMinutes(30))

}

@f-ranieri
Copy link
Contributor

I am also facing this issue. Additionally, the cache managers created in LocalResponseCacheGatewayFilterFactory are not visible in '/actuator/caches.' As I wanted the option to manually invalidate some of these caches from time to time, I ended up with a custom re-implementation of LocalResponseCacheGatewayFilterFactory. I don't know if there was a better option to achieve this.

@spencergibb
Copy link
Member

The properties are per cache manager. If you can describe how to do it without a cache manager per route, we'd be happy to work on it.

@spring-cloud-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@michael-wirth
Copy link
Author

The properties could be set per cache not per cacheManager as it is possible with any other spring cache manager. I also don't understand why the implementation is bound to Caffeine and not using a generic solution and support any Cache implementation.

To instantiate a cache manager for each LocalResponseCacheGatewayFilterFactory is a code smell and I think it should be possible to share a cache manager and not instantiate a new one each time.

@spencergibb
Copy link
Member

The properties could be set per cache not per cacheManager as it is possible with any other spring cache manager.

All the properties are set on Caffeine, not the cache manager. Can you show me how you would do it per cache?

public static CaffeineCacheManager createGatewayCacheManager(LocalResponseCacheProperties cacheProperties) {
Caffeine caffeine = Caffeine.newBuilder();
LOGGER.info("Initializing Caffeine");
Duration ttlSeconds = cacheProperties.getTimeToLive();
caffeine.expireAfterWrite(ttlSeconds);
if (cacheProperties.getSize() != null) {
caffeine.maximumWeight(cacheProperties.getSize().toBytes()).weigher(responseCacheSizeWeigher());
}
CaffeineCacheManager caffeineCacheManager = new CaffeineCacheManager();
caffeineCacheManager.setCaffeine(caffeine);
return caffeineCacheManager;
}

See #3145 for a more generic solution beyond caffeine.

@spencergibb
Copy link
Member

After looking, it looks like registerCustomCache() would be the way to go?

@github-project-automation github-project-automation bot moved this from In Progress to Done in 2023.0.1 Mar 19, 2024
@michael-wirth
Copy link
Author

Thanks, I wasn't aware of this chsmge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

No branches or pull requests

7 participants