Support store-by-value in ConcurrentMapCacheManager [SPR-13758]

[spring-projects-issues]

member sound opened SPR-13758 and commented

If you run the following code, the @Cachable method will not always return the same results (but it should).

@RestController 
@RequestMapping("/rest")
public class TestRunner {
	@Autowired
	private TestController controller;
	
	@ResponseStatus(HttpStatus.OK)
	@RequestMapping(value = "/test", method = RequestMethod.GET)
        @PostConstruct
	public void testCaching() {
		controller.test();
		controller.test();
	}
}

public class TestService {
	@Cacheable("test")
	public Set<String> get() {
		Set<String> set = new HashSet<>();
		set.add("ASD");
		set.add("123");
		return set;
	}
}

@Controller
public class TestController {
	@Autowired
	private TestService testService;

	public void test() {
		Set<String> list = testService.get();
		Assert.assertEquals(2, list.size()); //fails when run the 2nd time with: junit.framework.AssertionFailedError: expected:<2> but was:<1>

		Iterator<String> it = list.iterator();
		it.next();
		it.remove(); //removes the first item
	}
}

@Bean 
public CacheManager cacheManager() { 
     return new ConcurrentMapCacheManager(); 
}

Steps to reproduce the error: run http://localhost:8080/rest/test

I noticed that if I use a @PostConstruct method running the controller.test() method twice, this would not cause the error. So the application starts up fine, but invoking the servlet explicit triggers the failure.

---

Affects: 4.2.3

Issue Links:

• ConcurrentMapCacheManager's storeByValue does not pick up ClassLoader [SPR-14314] #18886 ConcurrentMapCacheManager's storeByValue does not pick up ClassLoader

Referenced from: commits cf20308, 0194988

[spring-projects-issues]

Juergen Hoeller commented

We're currently caching the result as-is. If your code decides to modify a returned collection there, it essentially breaks the @Cacheable contract.

That said, we could change that contract to always return a copy of the collection from any @Cacheable method declaring a collection return type. I suppose you'd actually expect a copy, right, not an unmodifiable collection?

In any case, this looks like a limitation of the current caching contract that you're violating through an explicit remove call. For the time being, you'll have to either use other means of caching or take a manual copy yourself before modifying the collection.

Juergen

[spring-projects-issues]

member sound commented

Ok, then indeed I might have misunderstood the @Cacheable concept. Basically I'd expect a method annotated to always return the same result for the same input parameters. No matter what happens before and after the cacheable call.

So you're right, I expected a copy of the list.

[spring-projects-issues]

Stéphane Nicoll commented

I don't think it's a good idea that @Cacheable changes the semantic of your return type all the sudden. Consider that piece of code that would work in a certain way without caching (i.e. it removes the underlying collection that you expose) but does not as soon as you enable caching (because the abstraction takes a copy). Or the other way around if you didn't expect the underlying collection to be modified.

The actual issue is not in @Cacheable but in the cache manager that you are using. The simple concurrent hash map implementation stores values as is but most cache libraries serializes the data (typically for clustered scenario). If you switch to any JSR-107 provider, your test will pass because store-by-value is the default.

There is no bug here IMO and I'd be tempted to close it but we could also offer an option to serialize data in ConcurrentMapCacheManager.

[spring-projects-issues]

Juergen Hoeller commented

Good point, it's actually a consequence of serializing a value versus storing it as-is and therefore dependent on the cache provider. Semantically, I'm not sure it's ever a good idea to modify a value returned from a cacheable method... But indeed, a store-by-value mode in ConcurrentMapCacheManager would make sense for such scenarios.

Juergen

[spring-projects-issues]

Stéphane Nicoll commented

ConcurrentMapCacheManager has now a setStoreByValue that one can enable to serialize the value instead of the value itself.