SPR-14235 - Reactive types support for @Cacheable methods

[padilo]

Support for cache Observable return values.

Not ready to be merge as it doesn't follow the Code style, but I need a bit of feedback :)
Tests are included

https://jira.spring.io/browse/SPR-14235

[snicoll]

Interesting, thanks!

ping @sdeleuze

[sdeleuze]

Interesting use case indeed, especially with Spring Framework 5 in mind.

As noticed in the JIRA issue, the first need here is to make Spring @Cacheable method handling aware of async errors that could be materialized as an error callback rather than an Exception directly thrown by the method. I think such need also apply to Java 8 CompletableFuture, Spring ListenableFuture and Reactor Mono.

About the implementation provided in this PR, if I understand correctly the data emitted by an Observable are stored as an ArrayList and wrapped/unwrapped from/to Observable. I think I have some concern with doing that. How that will behave with infinite streams? Isn't there some overlap between Spring @Cacheable method and RxJava .cache() operator? What about transforming a potentially async Observable into a blocking Iterable?

I have in mind some good use cases where supporting async single return values like Single, CompletableFuture, ListenableFuture and Mono on @Cacheable methods, but I am not sure how to handle stream caching with Observable, Flux or Stream, and not sure it makes sense to support/promote that.

Any thoughts?

[padilo]

With cache operator you can retrieve the events without replaying the business logic, but you can't add those events to using the spring cache as it isn't serializable, so like Optional you have make it serializable somehow. Also using the cache operator you will have a back-pressure problem as it will store it in memory, using infinite streams of data will be a problem.

The cache operator, as I understand, it doesn't provide a mechanism to add events to a persistent cache, I think Spring can help here.

Yes, the implementation uses an ArrayList to store the retrieved events as it's retrieved, it has some benefits doing this way, at it can be retrieve from the cache as a Iterable or whatever async implementation as long as it has a Wrapper implementation.

About infinite streams, I don't think spring cache should support it, as if we store the values it will full the cache, and if we store the logic between events it will force the classes involved to be Serializable and potentially many more race conditions (ThreadLocal, etc...). Also if it has to execute business logic it will not be a cache at all.

With current implementation it should be quite straightforward to implement it for other async wrappers, but maybe it need some change to fit in all cases or to be more understandable. :)

[bclozel]

Hi @padilo
Thanks for this very interesting PR.
Indeed with Spring 5.0, we're right in the middle of discussing how certain concepts can be translated in the reactive world. @Transactional and @Cacheable are examples of those.

The problem with those features is that not all concepts can be translated in the reactive world. Which means that we have to carefully review the big picture before committing to get @Cacheable working with reactive streams.

Alongside the questions asked by @sdeleuze we may wonder how keys should be generated if Observable, Flux or Mono types are used in the method signature or if the very blocking nature of cache stores may not be a big problem and break assumptions and contracts.

In all cases, I think this belongs to a broader issue we have to tackle in Spring 5.0.
I'm sure the whole team would like to discuss this in a JIRA ticket — I'm not aware of an existing one at the moment, I'll defer to @snicoll and @jhoeller since they're in charge of that part.

[padilo]

I understand and I'm completely agree. This kind of feature should fit very well with current needs in Spring 5.0 and having in mind the whole picture, this is not an easy task and should be take with very care.

Thank you very much for your feedback :)

[testark]

Is it in your roadmap to merge this anytime soon?

[snicoll]

There is a reference to a Jira issue. We'll work on that as part of Spring 5

[snicoll]

How that will behave with infinite streams?

@sdeleuze that does not make any sense to me. IMO, the only thing we should really support is Mono (even if we have Mono<List<X>>). The result of the method invocation is going to be cached and the cache abstraction has no special support for collections (if you return a collection, we cache that the exact same way as we cache a single pojo).

An infinite stream means you're trying to cache a collection of infinite size. Something is wrong elsewhere if you're trying to do that.

I can see some issue with the SpEL support (especially the one that has to run upfront). But if we limit ourselves to Mono with the ReactiveAdapter we should be fine.

@padilo would you be willing to rebase your PR and rework the code to use ReactiveAdapter rather than your current solution? That would allow to transparently support more return types. I'd only support Mono for now.

[padilo]

Sure I'll try to address this today. Still WIP to move to ReactiveAdapter.

[pivotal-issuemaster]

@padilo Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@padilo Thank you for signing the Contributor License Agreement!

[padilo]

Still pending some formatting errors, import order and so on (I'll correct this on another commit, probably tomorrow).

• Rebased on top of master
• Removed rx.Observable support.
• Added Mono support with some tests.
• Also added rx.Single using ReactiveAdapter to convert to Mono. (can be removed if not required right now).

Currently, if I haven't misunderstood you, we won't support Flux in favor of Mono<List<T>>

@snicoll I'm not sure if this is what you mean by use ReactiveAdapter. It would be very nice to have a ReactiveAdapterRegistry separating Mono from Flux adapters, because this way I didn't have to register each Mono equivalent class of each reactive library, and It would be more straightforward to use the ReactiveAdapter.

Another point I found is that there is a difference with Mono and Single:

• Mono doesn't accept a null event, Single does.
• Mono can have zero events. Single doesn't.

In the conversion using ReactiveAdapterRegistry It throws an error when you try to convert a Single with nulll event to Mono. There is an ignored test in CacheRxJavaTests.nullValue because of that.

I'll work on having the formatting 100% ok, while wait for feedback.

[snicoll]

Wow that's awesome! I can't reply you right the way regarding ReactiveAdapter as I haven't researched how to use it yet since you're doing my job ;-)

My intent was to look how the reactive controller (@RequestMapping) deals with return types: I know they have a single reactive "kind" internally with something doing the translation in case a user has selected a different reactive type. Maybe @rstoyanchev could help us?

[padilo]

Rebased on top of master and fixed import order, should be ok now.

@snicoll Thank you ;). I'll take a look to the @RequestMapping reactive handling.

[padilo]

Not sure if we can use ReactiveAdapter.getDescriptor().isMultiValue() to only cache reactive single event classes, instead of having to register rx.Singleon CacheResultWrapperManager.

If done this way I think it would work transparently and any new conversion that the ReactiveAdapterRegistry has, it would be supported by Spring Cache without having to explicitly register it, and the need to add the optional dependency. Taking a look at ReactiveAdapterRegistry we would have CompletableFuture support too.

I'm not 100% sure, but it sounds good in my head :)

[snicoll]

That's sort of what I have in mind. Let me go back on this one once I am done with other things and we can resume the discussion at that point. Thanks a lot for all your work!

[padilo]

Ok, thank you ;)

[padilo]

Rebased on top of master, improved ReactiveAdapter integration. Now all single events reactive implementation registered on ReactiveAdapterRegistry are supported, now is much simplier.

There isn't support to Mono<Optional<?>>, I think Mono can emit an event or don't emit anything, so it's not needed at all. But thinking on another single event implementations as Future or Single It can have sense..
Some ignored tests are on ReactorTests related to that. To support it I think some additional attributes are needed on CacheOperationMetadata, but maybe this PR should be merged before trying to solve this optional reactive case.

[snicoll]

@padilo thank you so much for the PR: it really helped to look at something concrete with regards to the feedback we get on Spring 5. At this point, I am afraid that the implementation does not match where we're heading.

If we add support for this, we need to be reactive all the way so we need a ReactiveCache with a Mono return type for the low-level cache operations. Also, @Cacheable has some features that do not make much sense in this new programming model so we'll probably need a dedicated annotation as well.

It looks like we're targeting the support of Mono only for now, as we've already discussed. But we definitely need to support reactive parameters as well and defer the computation of the key when such parameters are available.

We're still investigating if actual implementations of this ReactiveCache will be provided by various caching libraries. Also, JSR-107 isn't reactive for the moment so we won't be able to use it in that mode (one more reason to use a separate annotation).

I am now trying to build a test suite of what the behaviour should be and take it from there. I'll report on the Jira issue you've created.

[padilo]

Thanks @snicoll for all your time and your feedback 😉 It was fun to implement it and to look how to fit it in Spring Cache. And also I learn something about Reactor. 👍