spring-cloud / spring-cloud-gateway

An API Gateway built on Spring Framework and Spring Boot providing routing and more.
http://cloud.spring.io
Apache License 2.0
4.5k stars 3.31k forks source link

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

Closed michael-wirth closed 5 months ago

michael-wirth commented 1 year ago

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?

https://github.com/spring-cloud/spring-cloud-gateway/blob/eb098a759634f5e4898eb0963b83b966ae1b0063/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheGatewayFilterFactory.java#L75

ilozano2 commented 1 year ago

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

michael-wirth commented 1 year ago

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 commented 12 months ago

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 commented 12 months ago

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

alimodares2003 commented 12 months ago

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 commented 12 months ago

That is what I'm saying.

The gatewayCacheManager Bean is NOT used in the LocalResponseCacheGatewayFilterFactory! Please check the code at https://github.com/spring-cloud/spring-cloud-gateway/blob/eb098a759634f5e4898eb0963b83b966ae1b0063/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheGatewayFilterFactory.java#L75

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

alimodares2003 commented 12 months ago

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

mohhmekk commented 10 months ago

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 commented 8 months ago

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 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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?

https://github.com/spring-cloud/spring-cloud-gateway/blob/7e4bb5c185772021c8cb51a45543e7466cc0256c/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/config/LocalResponseCacheAutoConfiguration.java#L95-L107

See #3145 for a more generic solution beyond caffeine.

spencergibb commented 5 months ago

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

michael-wirth commented 5 months ago

Thanks, I wasn't aware of this chsmge!