spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
55.8k stars 37.84k forks source link

`sync=true` on `@Cacheable` looses reactor's Context #33079

Closed Crystark closed 1 week ago

Crystark commented 1 month ago

Affects: 6.1.8


Describe the bug @Cacheable annotation with sync=true on a reactor publisher returning method looses the reactive context

To Reproduce

This code reproduces the issue:

import com.github.benmanes.caffeine.cache.Caffeine
import org.springframework.cache.annotation.Cacheable
import org.springframework.cache.caffeine.CaffeineCache
import org.springframework.context.annotation.Bean
import org.springframework.context.annotation.Configuration
import org.springframework.scheduling.annotation.EnableScheduling
import org.springframework.scheduling.annotation.Scheduled
import org.springframework.stereotype.Component
import org.springframework.stereotype.Service
import reactor.core.publisher.Mono
import reactor.util.context.Context
import java.util.concurrent.TimeUnit
import kotlin.random.Random

@EnableScheduling
@EnableCaching
@Configuration
class ZeConfig {
    @Bean
    fun myCache() = createCache("MyCacheName")

    private fun createCache(name: String) = CaffeineCache(
        name,
        Caffeine.newBuilder()
            .maximumSize(100)
            .removalListener<Any, Any> { key, _, cause ->
                println("Removal of $key: $cause")
            }
            .buildAsync(),
        false
    )
}

@Service
class ServiceWithCache {
    @Cacheable("MyCacheName", sync = true)
    fun get() = Mono.deferContextual { context ->
        println(context)
        // ... do something
        Mono.just(Random.nextInt(1000))
    }
}

@Component
class Example(private val serviceWithCache: ServiceWithCache) {
    @Scheduled(fixedRate = 5, timeUnit = TimeUnit.SECONDS)
    fun process() {
        val it = serviceWithCache
            .get()
            .contextWrite(Context.of("some-thing1", "lala1"))
            .block()
        println("Result: $it")
    }
}

Actual behavior

Code execution prints the following

Context0{}
Result: 242
Result: 242
Result: 242
Result: 242
Result: 242

Expected behavior Code should print

Context1{some-thing1=lala1}
Result: 242
Result: 242
Result: 242
Result: 242
Result: 242

Workaround If I remove the sync=true it all works as expected so the issue comes from the use of sync=true

This was originally a question on stackoverflow.

simonbasle commented 2 weeks ago

I could not reproduce this as is. If you'd like us to spend some time investigating, please take the time to provide a complete minimal sample (something that we can unzip or git clone, build, and deploy) that reproduces the problem. Another option is some form of integration test that can be easily added to Spring Framework's test suite).

It would be interesting to try and avoid the scheduling aspect, in order to ensure there are as little moving parts as possible.

spring-projects-issues commented 1 week 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.

Crystark commented 1 week ago

Hi, sorry for my late answer @simonbasle

Thanks for getting back to me.

It seems I have missed in my example the @EnableCaching annotation. Maybe that's why you couldn't reproduce the issue ? Once added I could successfully make this work in a standalone app.

You'll find a maven project here that reproduces the issue. I didn't remove scheduling due to the lack of time on my end. I hope it's sufficient.

example.spring.issue33079.zip

simonbasle commented 1 week ago

Thanks for the reproducer, @Crystark, this provided good context for deeper analysis. Unfortunately, we cannot support context propagation in the case of sync=true.

Think of it that way: in order to avoid any double invocation of the service method, it is not your controller which subscribes to the mono it returns. Instead, it is the CaffeineCache which does (by converting the Mono to a Future, which subscribes to it without passing any context).

Imagine two concurrent calls to the cached service, each with a different contextWrite. With sync=false both calls fall through and the actual service method ends up being called twice with both contexts. With sync=true, it becomes undeterministic which of the two callers would "win" and instead the cache itself invokes the method, effectively passing the cached result to both callers.

Hope this helps.

Crystark commented 1 week ago

Yes I understand. Thanks for the investigation