ZiggyCreatures / FusionCache

FusionCache is an easy to use, fast and robust hybrid cache with advanced resiliency features.
MIT License
1.65k stars 87 forks source link

Explicitly tell the Factory that it failed #203

Closed chrisbbe closed 1 month ago

chrisbbe commented 6 months ago

Problem

Today, the only way to express that the factory fails is to throw an exception in the factory. Another approach in the spirit of functional programming is to use a Result monad to explicitly tell FusionCache that the factory failed.

Solution

Let's say we have a factory signature like this:

Task<Result<Data?>> GetMyDataAsync();

it would be nice and clean to avoid throwing an exception and do something like this:

_fusionCache.GetOrSetAsync<IStatus<Data?>>("foo", async (ctx, _) =>
 {
     var result = await GetMyDataAsync();
     ctx.FactoryFailed = result.IsValid == false; // Or something similar.
     return result;
});

achieving the same flow as throwing an exception. Does that sound reasonable?

jodydonetti commented 6 months ago

Hi @chrisbbe and thanks for using FusionCache.

I suppose you are referring to the old adage "don't use exceptions for flow control", which is something I really agree with, but here it's different (imho): FusionCache is not asking to throw an exception to control the flow, it is just handling a problem (normally happening via an exception being thrown) and doing extra activities, when possible, to better handle that.

But I can suppose then an observation like "ok, but still I would like to tell FusionCache explicitly about an error in a controlled way without throwing an exception" : and yes, I think can be done in theory with something like return ctx.Fail() similarly to what it can already be done when using Conditional Refresh via return ctx.NotModified() and return ctx.Modified(...).

But the problem then becomes: what should GetOrSet(...) return then?

Because if fail-safe is enabled for that call and there's a stale value to use then FusionCache of course will return that, but otherwise what should it return?

The only thing would be to... throw an exception, I think, and we would be back to square one, so to speak.

Am I missing something here? Do you have any other idea?

Thanks!

chrisbbe commented 6 months ago

As shown in my pseudocode, Result<T> is cached, and since the factory returns Result<T>, GetOrSet(..) then returns Result<T>, in the end it's up to the developer to define the signature like they do today, eventually make the return type nullable if they choose to go for the ctx.Fail() (and is forced to return something) method instead of throwing an exception.

jodydonetti commented 6 months ago

I have a couple more things to clarify about the approach, since there's some nuance (at least on my part to understand it clearly) that I need to better understand.

Let's say it's the first call (so, the cache is empty), and we do this:

var foo = _fusionCache.GetOrSetAsync<Result<T>>("foo", async (ctx, _) =>
{
  var result = await GetMyDataAsync();

  if (result.IsValid)
    return result;

  return ctx.Fail();
});

In this case ctx.Fail() should tell FusionCache that there has been an error, and that it should try to activate fail-safe and temporarily re-save and return an expired value.

But since the cache is empty, what should the ctx.Fail() do internally and what should it returnto the original caller of the GetOrSet() method?

Thanks!

celluj34 commented 5 months ago

We do something similar already - we're using FluentValidation as a sort of Result<T> that can bring along any error messages. In our case the factory is wrapped with try/catch inside GetOrSet and we use the special "do not cache" rules for 0 timeout so that we don't keep bad data in the cache (i.e., try again on next call).

If you'd like more detail on our use case, let me know, but this is something we work around currently.

celluj34 commented 5 months ago

This might be overstepping the bounds of any caching lib (idk I've never written one) but maybe having FusionCache return its own Result<T> would be a viable alternative?

jodydonetti commented 5 months ago

After some time with the idea marinating in the back of my head, and while I'm having a nice dinner with friends and some wine, I think I may just have had an idea...

Will update soon 🍷

jodydonetti commented 5 months ago

Coming back to this.

So I thought about 2 different but similar solutions (not a single line wrote, mind you) and they would be something like the following.

Option 1

Here my stream of consciousness:

Some other notes about this possible design: all the available overloads for GetOrSet<TValue> method would also be available for the new TryGetOrSet<TValue> method, so again everything will be uniform and the expectations of users when switching from one to the other would remain intact.

Of course also async versions available (eg: TryGetOrSetAsync<TValue> etc).

Regarding the factory signature: I'm thinking about not creating a new one where the result type is MaybeValue<TValue> instead of TValue, because even though at first it may makes sense, I prefer to be able users to just have to deal with one signature for the factory, and not 2 different ones.

So the next question of course is "how a factory will be able to tell FusionCache that there has been a fail without throwing an exception?", and for the (current, tentative) answer is by using a new method on the factory context object like they already exist for Modified(...) and NotModified(), and I'll take care of returning something that makes sense internally + signaling FusionCache that it has been "as if" an exception had been thrown.

FusionCache will take care of handling both cases transparently, meaning both factories that throw and that don't throw will be usable inside of both GetOrSet<TValue> and TryGetOrSet<TValue>, in a uniform way. When using the factory that doesn't throw with the current GetOrSet<TValue> method, FusionCache will take care of throwing an exception to align that with the normal flow there.

Example:

var maybeProduct = cache.TryGetOrSet<Product>(
  "product/123",
  (ctx, _) => {
    var product = GetFromDb(123);

    if (product is null) {
      return ctx.Fail();
    }

    return product;
  }
);

if (maybeProduct.HasValue) {
  // WORK WITH maybeProduct.Value ...
}

A similar example based on @chrisbbe original request would be something like:

var maybeData = await _fusionCache.TryGetOrSetAsync<Data?>(
  "foo",
  async (ctx, _) =>
  {
    var result = await GetMyDataAsync();

    if (result.IsValid == false) {
      return ctx.Fail();
      // OR MAYBE THIS
      //return ctx.Fail("Error message which will be logged, instead of the exception");
    }

    return result.Data;
});

if (maybeData.HasValue) {
  // WORK WITH maybeData.Value ...
}

Option 2

Basically same as above, but when calling a TryGetOrSet<TValue> method, the factory signature should be one with a MaybeValue<TValue> return type, basically ending up potentially having 2 different factory signatures to handle.

Thoughts?

What do you think? Again, I haven't started coding anything, so I'm very open to any input.

Thanks!

jodydonetti commented 5 months ago

Hi all, any opinion on my 2 proposals?

celluj34 commented 5 months ago

Whoops sorry, missed the previous notification.

I think I prefer option 1. In our particular use-case, we return our specific wrapper type because it's convenient. Incidentally, the ability to return any errors back through the cache "barrier" is a plus. We may be coding around some intended usage though, so maybe in the future we'll move the try/catch to outside GetOrSetAsync instead of inside. Then we could sidestep the entire issue.

But if you are considering this feature:

But since the cache is empty, what should the ctx.Fail() do internally and what should it returnto the original caller of the GetOrSet() method?

If there's a failsafe, keep returning that data. If there's not, then it's up to the caller to determine what to do next (most likely return some error that data couldn't be loaded). We have to do this anyway right now, so I don't think it really changes all that much from a client perspective.

jodydonetti commented 1 month ago

Hi all, after some considerations I decided to implement fail-safe without exceptions, here's the issue.

This should solve the original issue highlighted by @chrisbbe , while I'll take some more time to think about the possible new method TryGetOrSet<T>, since it was more of an evolution of the original request and it will take some more time to bake properly.

It's coming in the next version, v1.3.0, which will be out... in a couple of hours 😬

Hope this helps.

jodydonetti commented 1 month ago

Hi all, v1.3.0 is out 🥳

jodydonetti commented 2 weeks ago

Hi @chrisbbe , just wondering if you have been able to try this out.