[FEATURE] Cache miss on deserialization error in L2 cache

angularsen commented 4 weeks ago

Problem

On a few occations I have accidentally broken cache deserialization by adding non-nullable properties to the DTO class for the cache entry, while existing entries in Redis are missing these. This mistake is easy to make, and it can effectively cause downtime for an API resource attempting to read the cache entry with GetOrSet semantics.

Solution

Add option, e.g. FusionCacheEntryOptions.TreatDistributedCacheDeserializationErrorAsCacheMiss, to treat it as cache miss
- For GetOrSet, it will invoke the factory method
- For Get, it will return no result

I first considered to evict the distributed cache entry, but I realize that may add a performance hit and it may be better to just treat it as a cache miss and only update the cache entry if the factory successfully produces a new fresh value.

Alternatives

Using Events to invalidate cache on deserialization error. However, this still results in one failed API request for each cache entry.

I want to achieve zero failed API requests if the cache entry is invalid for any reason, without having to try-catch everywhere or add my own extensions methods on top of GetOrSet.

fusionCache.Events.Distributed.DeserializationError += OnDistributedOnDeserializationError;

private static void OnDistributedOnDeserializationError(object? sender, FusionCacheEntryEventArgs args)
{
    if (sender is IFusionCache senderCache)
    {
        senderCache.Remove(args.Key);
    }
}

Additional context

None

I'm up for attempting a pull request on this, if that helps move this along.

jodydonetti commented 3 weeks ago

Hi @angularsen , this is a good one!

In theory it would also help when there are wire format versioning updates.

I have to think about it a little bit cause edge cases, potentially infinite update cycles and so on but... it may work!

Will update.

jodydonetti commented 3 weeks ago

ps: right now I'm deep into Tagging which is uber complex and very delicate (and if you'd like to spend some time reading the proposal and sharing you think it would be really helpful), but I'll tackle this one right after that.

angularsen commented 3 weeks ago

Great! Yes, I would think it helps a lot.

I just stumbled on the Tagging thread while posting this, and that also looks super interesting. Following closely, as I've struggled to find a good strategy to invalidate say all cache for a particular user, and your proposal looks like a real nice solution to that 👏

jodydonetti commented 1 week ago

Hi @angularsen I was just re-reading this issue and I thought that I haven't mentioned setting ReThrowSerializationExceptions to false.

Have you already tried it?

angularsen commented 1 week ago

@jodydonetti No I have not, and I'm not sure I understand how that would work based on the description. How can I use this to ignore or invalidate invalid JSON? Will it simply treat it as cache miss already, or will something else happen?

🧙 `ReThrowSerializationExceptions`	`bool`	`true`	Set this to false to disable the bubble up of serialization exceptions (default is `true`).

jodydonetti commented 1 week ago

I may expand the docs a little bit, but yeah: with false it would be as if there was nothing to deserialize, so basically a cache miss.

Now that I think about it though, it would be better to be able to differentiate between reads and writes: an error on read (deserialize) may be ignorable, but an error when writing would mean 99.999% of the times a bad configuration, which would not go away anyway unless fixed in code.

Should I add 2 separate options maybe? Thoughts?

angularsen commented 1 week ago

Is there any usecase for not rethrowing on write serialization error? Not sure when anyone would ever want that.

I see two options:

Rewrite the documentation for the existing property and let it only control reads, and let writes always fail.
Or remove/deprecate this one, and add a new property that is more explicit in its name, e.g. TreatDistributedDeserializationErrorAsCacheMiss, or RethrowDistributedCacheDeserializationException, or RethrowDeserializationException to let it count for any layer of cache.

Either way, it's kind of a breaking change for both default options and for users explicitly configuring ReThrowSerializationExceptions already.

jodydonetti commented 1 week ago

Is there any usecase for not rethrowing on write serialization error? Not sure when anyone would ever want that.

Well... I mean, I can't think of any right now 😑

Back when I added that option, I did so while also adding ReThrowDistributedCacheExceptions and ReThrowBackplaneExceptions so maybe I got carried away by making a parallel one as if the ones for the distributed cache and the backplane would be the same as the one for the serializer.

In fact there is already a difference between the first 2 and the last one: by default the first two are set to false (because they can be transient and we have Auto-Recovery to cover those errors) while the last one is set to true, because of the different nature of it.

You know what? Probably I can just... (see below)

I see two options:

Rewrite the documentation for the existing property and let it only control reads, and let writes always fail.

... this, I think it's the way to go. I'll think about it a bit more, but this is probably the right approach.

Either way, it's kind of a breaking change for both default options and for users explicitly configuring ReThrowSerializationExceptions already.

Two things here:

if there's no reason to suppress serialization exceptions (on write) but only de-serialization, in theory it's not actually a breaking change, since we just decided that there's no reason to disable the write ones
also yes, I understand that it technically is, but still: the change will come with v2

And v2 is a major version, which allows for breaking changes (and I'm already making some, although hopefully backward compatible for 99.999% of the cases).

So yeah I think we have a solution here.

Agree?

angularsen commented 1 week ago

Sounds great to me 👍 I'm unblocked already I guess since I don't have any issues with serialization on writes. I'll try disabling this and see if it achieves what I was asking for in this ticket.

angularsen commented 1 week ago

Verified that setting ReThrowSerializationExceptions = false does treat JSON deserialization error in distributed cache as cache miss 🎉

Closing this as solved, although the solution we discussed still makes sense 👍

jodydonetti commented 1 week ago

Verified that setting ReThrowSerializationExceptions = false does treat JSON deserialization error in distributed cache as cache miss 🎉

Awesome, happy you're unblocked now!

Closing this as solved, although the solution we discussed still makes sense 👍

Yep, I'll create an issue to track that change and will include it in v2.

ZiggyCreatures / FusionCache