alastairtree / LazyCache

An easy to use thread safe in-memory caching service with a simple developer friendly API for c#
https://nuget.org/packages/LazyCache
MIT License
1.71k stars 159 forks source link

Why it is not possible to add null items in the cache via IAppCache.Add ? #155

Open EnricoMassone opened 3 years ago

EnricoMassone commented 3 years ago

Can you please explain why adding a null item to the cache via IAppCache.Add is disallowed ?

I'm asking because the underlying ASP.NET core memory cache allows to add null items to the cache via IMemoryCache.Set.

Is this design choice intended to avoid a situation like the following one, where the semantic of the null reference is somewhat ambiguous ?

class Program
{
    static void Main(string[] args)
    {
      var options = new MemoryCacheOptions();
      var cache = new MemoryCache(options);

      cache.Set<string>("the-key", null, DateTimeOffset.UtcNow.AddDays(1));

      var item = cache.Get<string>("the-key");
      var otherItem = cache.Get<string>("other-key");

      // both item and otherItem are null references, but item has been retrieved from the cache
    }
}

Thanks for the clarification.

alastairtree commented 3 years ago

Exactly as you describe - if you allow someone to cache null, they cannot determine when doing a Get if the item was removed from the cache or if they cached null.

If you really want to do weird things with null then you should cache a Nullable<MyThing> and then it is explicit when you are deliberately caching a null.

EnricoMassone commented 3 years ago

Exactly as you describe - if you allow someone to cache null, they cannot determine when doing a Get if the item was removed from the cache or if they cached null.

If you really want to do weird things with null then you should cache a Nullable<MyThing> and then it is explicit when you are deliberately caching a null.

Thanks for the clarification, now your design decision is clear to me.

Sometimes caching null items can be useful. I know that using nulls as return values from methods is quite controversial, but in real-world code bases there are plenty of methods such as the following one:

public interface IProductRepository 
{
  // returns null if a product having the specified id doesn't exist
  Product GetById(int id);
} 

Maybe you want to cache the result of calling IProductRepository.GetById, even in the case when null is returned. The workaround of defining a null object totally makes sense to me. The only point that I have with this solution is that, in a certain sense, makes the usage of LazyCache a little more uncomfortable. The code without nulls is safer for sure, but a little more complex.

Maybe there is an alternative. The IAppCache interface defines the following method:

bool TryGetValue<T>(string key, out object value);

By using this method an item is fetched from the cache by key and the bool value returned indicates whether the item was available in the cache. In this case the semantic is clearer than the one in the IAppCache.Get, because there is an explicit boolean value to discriminate between a cache hit and a cache miss.

So a possible change can be allowing to store null values via IAppCache.Add and suggesting users to fetch items via IAppCache.TryGetValue in order to avoid ambiguity. There is probably the need to augment the IAppCache interface with a new TryGetValueAsync method to address the point described in #154. Maybe with such a change the two methods IAppCache.Get and IAppCache.GetAsync could be removed entirely to avoid any ambiguity for the API users.

The main point for this idea is that caching null values is disallowed when using IAppCache.Add, but at the same time you can endup caching null values when using IAppCache.GetOrAddAsync. So, basically, there is a mixture: in one case you cannot cache null items, but in the other one you can.

To summarize it's a tradeoff between having a safer API which tries to avoid working with null and an easier to use API, which allows the user to choose whether or not working with null. I totally agree that relying on null to express a semantic is a bad practice, but at the same time we still not have a simple tool to completely eradicate it from the language.

alastairtree commented 3 years ago

Good point about being able to cache a null with the GetOrAdd methods, that is also inconsistent. As you say managing nulls is always imperfect in c#due to the language design although recent null annotations do make it better.

Part of me wonders if it's better to give up and let developers just cache a null if they really want to, and remove the null check in Add. What do you think?

I have to admit I never use the Get, TryGet or the Add methods, I only ever use the GetOrAdd flavour since I want my operation to only execute once which is the main benefit of the package, so that's probably why I haven't run into it before.

EnricoMassone commented 3 years ago

Unfortunately the whole concept of null reference is deeply rooted into the language. I have seen various examples of functional libraries attempting (among the other things) to eradicate the null reference from the language, but all of them add a lot of burden. I tend to prefer simplicity.

I wrote myself a simple wrapper class attempting to remove the usage of null as a way to express the absence of a result. But again this adds burden: to be honest after migrating to .NET core 3.1 I prefer using nullable reference types instead of my own wrapper.

To summarize, I would allow null values to be cached via IAppCache.Add.

You can also consider a couple of changes to IAppCacheinterface:

All of these is a breaking change of the public API, so I think it goes to the roadmap for the next major release. Maybe I can help with a pull request if you think that all of this makes sense.

alastairtree commented 3 years ago

Yeah that all makes a lot of sense to me and would tidy things up - I would accept a PR based on those changes with supporting unit tests. Although technically breaking API changes, they are all actually very minor breaking changes - I doubt any of them would break users apps, and so I am not even sure they would warrant a major version.

EnricoMassone commented 3 years ago

Yeah that all makes a lot of sense to me and would tidy things up - I would accept a PR based on those changes with supporting unit tests. Although technically breaking API changes, they are all actually very minor breaking changes - I doubt any of them would break users apps, and so I am not even sure they would warrant a major version.

Hello, I have manged to fork the repo. I'll produce a single pull request containing all of the changes, is it fine for you ?

EnricoMassone commented 3 years ago

Hi @alastairtree did you take a chance to evaluate my pull request ?

jstutson-rel commented 10 months ago

Hi any chance this can get some more priority?

Just want to re-iterate NULLs are not going anywhere in C# as a representation for the absence of data. The new nullability features in newer c# versions are geared towards more null safety, not for any alteration in their meaning or frequency of use. They only ask for more consideration as to whether you really want to use NULL or should expect NULL at any given point.

paschalisTo commented 4 months ago

Hi, what is the status of this feature? I see there is a PR for almost three years now but no updates of its status.