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

Spring Data Redis Cache implementation is not compatible with Cache.retrieve(key) semantics #31637

Closed
yangwenliang123 opened this issue Nov 21, 2023 · 2 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug
Milestone

Comments

@yangwenliang123
Copy link

yangwenliang123 commented Nov 21, 2023

In the CacheAspectSupport.findInCaches method
CompletableFuture<?> cachedFuture = cache.retrieve(key);
spring cache result is null, spring redis cache result is CompletableFuture.completedFuture(null) causing result return error

@Nullable
public Object findInCaches(CacheOperationContext context, Cache cache, Object key) {
	ReactiveAdapter adapter = this.registry.getAdapter(context.getMethod().getReturnType());
	if (adapter != null) {
		CompletableFuture<?> cachedFuture = cache.retrieve(key);
		if (cachedFuture == null) {
			return null;
		}
		if (adapter.isMultiValue()) {
			return adapter.fromPublisher(Flux.from(Mono.fromFuture(cachedFuture))
					.flatMap(v -> (v instanceof Iterable<?> iv ? Flux.fromIterable(iv) : Flux.just(v))));
		}
		else {
			return adapter.fromPublisher(Mono.fromFuture(cachedFuture));
		}
	}
	return NOT_HANDLED;
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 21, 2023
@mp911de
Copy link
Member

mp911de commented Nov 21, 2023

Thanks for reaching out. I think you've spotted a shortcoming of the design. The current approach expects a null value for a cache miss and a CompletableFuture for any cache hit.

Arguably this approach can work for in-memory caches. Any cache that requires I/O to determine whether there is a cache entry (such as Redis) would be required to await Future completion making the async cache API a blocking code path.

It makes sense to revisit this arrangement with the team.

@snicoll snicoll added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 21, 2023
@jhoeller jhoeller added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 21, 2023
@jhoeller jhoeller added this to the 6.1.1 milestone Nov 21, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Nov 21, 2023

A side note: Reactive caching can also work with the @Cacheable(sync=true) mode of operation which delegates to the value loader based SPI method retrieve(key, Supplier<CompletableFuture>), restricted to a single cache and a synchronized retrieve+put step. Could you give that a try and see whether that kind of declaration works as expected with Redis?

As for the regular mode of operation, I've revised the cache interceptor implementation to be able to deal with Redis-style late-determined cache misses through an empty CompletableFuture as well. This will make it into the 6.1.1 release on Thursday, just in time for the Spring Boot 3.2 GA release. We'll make sure to test this with Redis tomorrow.

As a bonus, in order to support null cached values, such a late-determine retrieve(key) implementation can also return a CompletableFuture with a nested ValueWrapper, differentiating between null as a cache miss and null as a cached value. Spring's common processing can deal with all variants of these implementation strategies now.

@jhoeller jhoeller changed the title spring cache and spring redis cache Inconsistent design results Spring Data Redis Cache implementation is not compatible with Cache.retrieve(key) semantics Nov 21, 2023
jhoeller added a commit that referenced this issue Nov 22, 2023
Includes revised cacheNames javadoc and equals/hashCode for SimpleValueWrapper.

See gh-31637
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) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants