Support GetOrAdd and TryGetOrAdd for CacheItem

[ahockersten]

Fixes #152.

I have modified tests to cover the new code, but I have not run them, because I don't have redis setup on Windows at the moment.

I duplicated the helper functions used by the other code for the CacheItem version, as I couldn't figure out a way of rewriting the helpers that I was happy with. The obvious way would be to make sure they took a CacheItem and then make the other versions create one on the fly, but that seems a bit wasteful memory-wise especially since it often ends up being unused. Anyway, I'm leaving it up to you. If you want me to rewrite things some other way, by all means ask and I will do so.

[MichaCo]

Hi @ahockersten
To run redis locally, just run redis-server-master.cmd from tools/redis. This should download redis for windows and run it, or use linux subsystem for windows and apt-get install redis-server.

Regarding the implementation, I think the overloads for CacheItem should also have a value factory, as the purpose of those methods is lazy evaluation of the object creation in case it doesn't exist. Meaning, the factory gets called only if Get(key) doesn't yield any result.

In your implementation, you'd always have to provide a constructed cache item, even if it is not needed.

I think the interface should be more like

TCacheValue GetOrAdd(string key, string region, Func<string, string, CacheItem<TCacheValue>> valueFactory);

bool TryGetOrAdd(string key, string region, Func<string, string, CacheItem<TCacheValue>> valueFactory, out TCacheValue value);

(plus the corresponding overloads without region)

You could refactor the existing private bool TryGetOrAddInternal(string key, string region, Func<string, string, TCacheValue> valueFactory, out TCacheValue value) to accept the CacheItem func instead and use that one in the "old" methods, too.

Hope that makes sense and happy coding :>

[ahockersten]

True, there should be a ValueFactory version. I'm wondering if there should be a version that just takes a value as well, or if you think that's unnecessary?

[MichaCo]

I would say it is not necessary to have one without the factory

[ahockersten]

Actually, now I remember why I didn't add a ValueFactory version in my original commit. If you do, then you have to create CacheItems with no value set. That seems like it would encourage a potentially bad practice, as it would mean GetOrAdd() would have to modify the provided CacheItem if (and only if) it was added.

Consider this example:

var value = 1;
var value2 = 2;
var item = new CacheItem(key, value);
var cacheItem = cache.GetOrAdd(item, (k, r) => value2);

The case where there is already is an item in the cache is uninteresting, so let's not consider that.
The other case however, is: what does the GetOrAdd() return here, if there is no item in the cache? Does it return item' but modified so as Value == value2, or does it return a clone of the CacheItem?

Now that I have written out the question, it seems like the answer doesn't really matter, because CacheManager doesn't use CacheItems in a way that would make it matter (I think?), and if you are using CacheItems as anything other than an interface for communicating with CacheManager then you're on your own.

Did the above explanation make any sense, or am I mostly explaining away things to myself? :)

Btw, thank you for the Redis setup instructions.

[ahockersten]

Also, I should have read your interface proposal, as it neatly rounds the potential non-problem I described above. 🤦‍♂️

[MichaCo]

Hehe no worries, always good to talk about those things to get to a better understanding ;)

My interface suggestion had the key and region as parameters as that's what you'd need to do the Get and keep the CacheItem lazy (as you found out yourself, yup)

Regarding the return value, I guess it would actually make more sense to return the CacheItem instead of the value only => in case you need it, you don't have to make another call to GetCacheItem.

[ahockersten]

I think everything should be in order now. As you suggested, I return the CacheItem rather than the value for the functions that expect CacheItems to be passed into them.

[ahockersten]

Or so I thought, at least. Weird. Tests didn't fail on my computer last night. Will look at this when I get home tonight.

[ahockersten]

I found the remaining issues and fixed them.

[ahockersten]

Or so I thought anyway. Some tests are failing, but it seems that was a preexisting problem?

BTW, I made a change to the behavior of GetOrAdd(), which meant I had to make a change to the GetOrAdd_AddNull() test. GetOrAdd() now throws an ArgumentNullException if the key is null, previously it would throw an InvalidOperationException(). Technically this is a change in the API, I suppose.

I'll note that the docs say GetOrAdd() should throw an ArgumentException on invalid arguments, so documentation-wise it seems this behavior could be considered a bugfix.

[MichaCo]

@ahockersten I'm currently busy with other things, I will review it later this week

[MichaCo]

Looks good overall, just a few minor things

[MichaCo]

.Value will error with NullReference eventually, use ?.Value ?

[ahockersten]

AFAICT, there's no way the CacheItem returned from GetOrAddInternal() could ever be null? If a function generating null is passed into GetOrAddInternal() it will explicitly throw inside of it.

[MichaCo]

TryGetOrAddInternal can return null. It is very unlikely, but it can if retries is 0 and get and add fails for example.

Yeah right, in GetOrAddInternal it throws. You are right ;)

[ahockersten]

TryGetOrAddInternal can indeed return null in that case, but GetOrAddInternal cannot? It will always throw in the described situation.

[MichaCo]

yup, edited my comment => Yeah right, in GetOrAddInternal it throws. You are right ;)

[MichaCo]

same as above

[ahockersten]

Same as above. :)

[MichaCo]

same as above

[MichaCo]

The block can be simplified a little

if (TryGetOrAddInternal(
    key,
    null,
    (k, r) =>
    {
        var newValue = valueFactory(k);
        return newValue == null ? null : new CacheItem<TCacheValue>(k, newValue);
    },
    out CacheItem<TCacheValue> item))
{
    value = item.Value;
    return true;
}

value = default(TCacheValue);
return false;