FusionCache icon indicating copy to clipboard operation
FusionCache copied to clipboard

Explicitly tell the Factory that it failed

Open chrisbbe opened this issue 1 year ago • 9 comments

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?

chrisbbe avatar Feb 20 '24 22:02 chrisbbe

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!

jodydonetti avatar Feb 21 '24 22:02 jodydonetti

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.

chrisbbe avatar Feb 22 '24 10:02 chrisbbe

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!

jodydonetti avatar Feb 22 '24 14:02 jodydonetti

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 avatar Mar 22 '24 16:03 celluj34

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?

celluj34 avatar Mar 22 '24 16:03 celluj34

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 avatar Mar 22 '24 21:03 jodydonetti

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:

  • in FusionCache there is already a way to explicitly know if something was in the cache or not, thanks to the TryGet<TValue> method
  • that method returns a value of type MaybeValue<TValue>, which is kind like Result<TValue> or an option/maybe monad etc, so that can be the return value
  • the GetOrSet<TValue> method though returns directly a TValue, but that is correct! The method is called Get... and not TryGet..., so that makes sense and is uniform with the overall design
  • so the idea may be to have a new TryGetOrSet<TValue> method which would return a MaybeValue<TValue> instead of directly a TValue, so that everything would still be aligned again

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 avatar Mar 30 '24 14:03 jodydonetti

Hi all, any opinion on my 2 proposals?

jodydonetti avatar Apr 08 '24 19:04 jodydonetti

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.

celluj34 avatar Apr 08 '24 20:04 celluj34

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 avatar Aug 04 '24 20:08 jodydonetti

Hi all, v1.3.0 is out 🥳

jodydonetti avatar Aug 04 '24 22:08 jodydonetti

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

jodydonetti avatar Aug 23 '24 20:08 jodydonetti