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

Provide the possibility to invalidate a cached Uni #231

Closed
heubeck opened this issue Jul 24, 2020 · 20 comments
Closed

Provide the possibility to invalidate a cached Uni #231

heubeck opened this issue Jul 24, 2020 · 20 comments
Labels
bug Something isn't working enhancement New feature or request on-roadmap The issue is part of the roadmap
Milestone

Comments

@heubeck
Copy link
Collaborator

heubeck commented Jul 24, 2020

Hello Mutiny team,

it would be great if an Uni created by Uni.cache() could be invalidated to cause a resubscription on the original Uni.
I can imagine an overloaded version Uni.cache(Predicate<T> isValid) that is called on every subscription on the cached Uni and as soon as it gets false the cached item is thrown away in favour to compute a new one.

Thank you and keep on making our JVM world reactive 🚀

@heubeck heubeck changed the title Provide the possibility to invalidate an cached Uni Provide the possibility to invalidate a cached Uni Jul 24, 2020
@jponge
Copy link
Member

jponge commented Jul 29, 2020

Interesting, would you have a small snippet that would benefit from the feature?

Also how about cacheWhile(Predicate) as an alternative name?

@heubeck
Copy link
Collaborator Author

heubeck commented Jul 29, 2020

Interesting, would you have a small snippet that would benefit from the feature?

Also how about cacheWhile(Predicate) as an alternative name?

Yeah it's all about naming ;)

My concret use-case is fetching JWT tokens from an auth-service.
The response of that service can be used until the JWT expires, so the validity check of the cached Uni would be wether the token is still valid.

@jponge
Copy link
Member

jponge commented Jul 29, 2020

Interesting

@jponge
Copy link
Member

jponge commented Jul 30, 2020

@heubeck we're more or less all in vacations or going to vacations, so don't be surprised to not hear back anything for the next 2-3 weeks.

Meanwhile if you have time and fancy looking at what's under the cover, feel-free to hack and possibly open a pull-request 😉

@heubeck
Copy link
Collaborator Author

heubeck commented Jul 30, 2020

@heubeck we're more or less all in vacations or going to vacations, so don't be surprised to not hear back anything for the next 2-3 weeks.

Meanwhile if you have time and fancy looking at what's under the cover, feel-free to hack and possibly open a pull-request 😉

Already thought about doing this - hope to free some time.

Have a good time and thank you @jponge .

@heubeck
Copy link
Collaborator Author

heubeck commented Jul 31, 2020

@jponge suggested

Interesting, would you have a small snippet that would benefit from the feature?

Also how about cacheWhile(Predicate) as an alternative name?

wouldn't that pollute the Uni API?
If breaking changes are accepted I could try to refactor to a cache config similar you provide for most other API branches (please help me name it):

// current behaviour would become:
Uni.cache().indefinitely()
// Positive cache validity checking:
Uni<T>.cache().asLong(Predicate<T>)
// The other was around:
Uni<T>.cache().until(Predicate<T>)

If you do not want to enhance the caching stuff further, than I'm happy with Uni.cacheWhile(Predicate) side by side with Uni.cache().

Thank you for reading my thoughts, looking forward to implement a proposal.

@cescoffier
Copy link
Contributor

cescoffier commented Aug 1, 2020 via email

@heubeck
Copy link
Collaborator Author

heubeck commented Aug 4, 2020

I like the idea of having a cache group. asLong should be atMost to be homogeneous and receive a Duration. We could still keep the current cache method which would redirect on cache().indefinitely(). Le sam. 1 août 2020 à 00:21, Florian Heubeck notifications@github.com a écrit :

@jponge https://github.com/jponge suggested Interesting, would you have a small snippet that would benefit from the feature? Also how about cacheWhile(Predicate) as an alternative name? wouldn't that pollute the Uni API? If breaking changes are accepted I could try to refactor to a cache config similar you provide for most other API branches (please help me name it): // current behaviour would become: Uni.cache().indefinitely() // Positive cache validity checking: Uni.cache().asLong(Predicate) // The other was around: Uni.cache().until(Predicate) If you do not want to enhance the caching stuff further, than I'm happy with Uni.cacheWhile(Predicate) side by side with Uni.cache(). Thank you for reading my thoughts, looking forward to implement a proposal. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADCG7JDGXHJB2SLW5AREITR6M7VBANCNFSM4PG5E2SQ .

Thank you @cescoffier.
Can you please have a look at my implementation proposal at #240.
My need is to valided the cache based on the item itself.

Currently I have no idea how to provide a atMost(Duration) since I suppose the duration timeframe should start at time of onItem of the cached result, not when the UniCache itself is constructed.
I placed some questions in the PR itself, so maybe we can discuss about single issues separately.

@cescoffier cescoffier added the on-roadmap The issue is part of the roadmap label Aug 25, 2020
@jponge
Copy link
Member

jponge commented Sep 9, 2020

Looking into it, there is indeed an opportunity to explore another design. It's a more complex state machine if re-subscription happens. It's on my radar.

@jponge
Copy link
Member

jponge commented Sep 21, 2020

I've kept your PR open as draft for reference @heubeck

I've been exploring some designs with synchronized blocks but it's not great (but apparently correct). I'm going to have a look at drain loops and see if we can make it work this way.

Thinking out loud. On the other hand I'm wondering if we are not trying to ask too much for what Uni can do. A Uni represents an operation, and caching a result is perhaps something that should be done as part of an application business logic rather than in Uni itself. Invalidation can quickly become complex, and there are good libraries for that (e.g., https://github.com/ben-manes/caffeine).

@gavinking
Copy link

@jponge Don't think of it as "caching". A cache is a complex beast with explicit invalidation and timeouts and LRU eviction, etc.

Think of it as memoization.

@jponge
Copy link
Member

jponge commented Sep 21, 2020

@gavinking spot on, memoization is a more appropriate term here.

@jponge
Copy link
Member

jponge commented Sep 21, 2020

@gavinking Out of curiosity do you have examples where this would be useful in your usage of Uni?

@gavinking
Copy link

@jponge I have not found a use for it yet, but remember that so far we have not progressed much beyond just writing simple test cases for HR.

@heubeck
Copy link
Collaborator Author

heubeck commented Sep 21, 2020

@jponge Don't think of it as "caching". A cache is a complex beast with explicit invalidation and timeouts and LRU eviction, etc.

Think of it as memoization.

@gavinking thank you for introducing a better term.

I'd like to remind to my original case raising that invalidation requirement:
Fetching an auth token, that is valid for a certain period.

But I'm also with @jponge , maybe that's over-engineering within Uni.

I'd liked the idea of handling simple caching cases directly, but if it doesn't fit in the design, it can be left out and handled by an application itself.

Please do not waste time in there if you are not confident about its usefulness.

I had fun trying to solve it, but there surely will come up more handy challenges to implement.

@jponge
Copy link
Member

jponge commented Sep 21, 2020

@heubeck I'm still willing to explore and see if we can come up with a good solution here. Right now cache() provides us with memoization. Supporting invalidation (like a Clojure library does) is a good investigation area.

@heubeck
Copy link
Collaborator Author

heubeck commented Sep 30, 2020

@jponge just a little idea:
If the validity of a cached/memorized Uni is defined only time-based, and no Predicate determines the validity but only a Duration, a probabilistic early expiration could mitigate the race conditions.

If solving the race conditions just for this case is too complicated or courses high synchronization overhead it's propably not worth it.

@jponge
Copy link
Member

jponge commented Sep 30, 2020

@heubeck Noted. I've drafted an alternative design for UniCache in #312, I will next see if it can cater for some invalidation logic.

@jponge
Copy link
Member

jponge commented Sep 30, 2020

@heubeck I pushed an early sketch / draft in #313

It provides cacheUntil(BooleanSupplier) and cacheFor(Duration).

Note that in this design invalidation is checked on new subscriptions, it looks quite sane to reason about IMHO. If we start also checking when and item or failure is ready to be propagated downstream then we may have starvation / infinite loops, especially in the cacheUntil case if the condition changes on the same thread.

@jponge jponge linked a pull request Oct 19, 2020 that will close this issue
@jponge jponge removed a link to a pull request Oct 19, 2020
@jponge jponge added this to the 0.10.0 milestone Oct 21, 2020
@jponge
Copy link
Member

jponge commented Oct 21, 2020

Done in #312, superseded by #313

@jponge jponge closed this as completed Oct 21, 2020
@jponge jponge added enhancement New feature or request bug Something isn't working labels Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request on-roadmap The issue is part of the roadmap
Projects
None yet
Development

No branches or pull requests

4 participants