1
\$\begingroup\$

Here I am again; referring to my previous code review request about a self-updating, asynchronous (?) cache.

The previous class takes care of caching operations like "get all users registered in our basecamp company", and updates the list through a getter function, periodically through a TimeSpan parameter supplied at instantiation. Then, Get(TId) and GetAll() methods are supplied to provide access to the gotten (?) objects. Note that I haven't updated the questions' code after the feedback provided. (for those interested: here)

This class, on the other hand, is for caching individual objects rather than an object bulk. For example, when having clicked on a basecamp project in my application, I need to map it's members to own registered user, look for a google folder, calculate vertain economic KPI's, amongst other things. This is only to be done when the consumer can provide the project Id, since certain API's restrict their "get all" methods to certain parameters, for example "updated since" or even restricting to a number of results - so a complete list of objects may be impossible to retrieve.

The class only supplies one provision method, Get(TId id), and blocks execution if the cache doesn't contain the corresponding object. If it does contain it, it is returned. Meanwhile, the ActionBlock awaits the task of updating the instance again, updating it when time comes. When this is done, all calls to Get(TId id) (using the same id, of course) yields the new instance.

Are there any concurrency issues, memory leaks, undesired behavior of this approach?

I see these cache classes as "alive" which lead me to naming them OrganicCache. Extending this functionality further could include caching multiple bulks by parameters, or even making generic what you're caching (individual objects or collections of objects?) and what you're using as parameters (id, time, ..). Merging these classes into one would be an interesting endeavour.

It uses ActionBlock<> from the TPL Dataflow library (nuget it).

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Threading.Tasks.Dataflow;

namespace OrganicCache
{
    public class IndividualisticCache<TInstance> : IndividualisticCache<string, TInstance> {
        public IndividualisticCache(Func<TInstance, string> idFunction, Func<string, TInstance> getterFunction, TimeSpan getterFunctionWaitPeriod) : base(idFunction, getterFunction, getterFunctionWaitPeriod) { }
        public IndividualisticCache(Func<TInstance, string> idFunction, Func<string, TInstance> getterFunction) : base(idFunction, getterFunction) { }
    }

    public class IndividualisticCache<TId, TInstance>
    {
        public IndividualisticCache(Func<TInstance, TId> idFunction, Func<TId, TInstance> getterFunction, TimeSpan getterFunctionWaitPeriod)
            : this(idFunction, getterFunction)
        {
            _getterFunctionWaitPeriod = getterFunctionWaitPeriod;
        }

        public IndividualisticCache(Func<TInstance, TId> idFunction, Func<TId, TInstance> getterFunction)
        {
            _getterFunction = getterFunction;
            _idFunction = idFunction;
        }

        private readonly Func<TId, TInstance> _getterFunction;
        private readonly Nullable<TimeSpan> _getterFunctionWaitPeriod;

        private readonly Func<TInstance, TId> _idFunction;

        private readonly Dictionary<TId, bool> _hasRunGetterFunctionFirstTimeForInstance = new Dictionary<TId, bool>();
        private readonly Dictionary<TId, object> _hasRunGetterFunctionFirstTimeForInstanceLock = new Dictionary<TId, object>();

        private readonly ConcurrentDictionary<TId, TInstance> _instances = new ConcurrentDictionary<TId, TInstance>();
        private readonly object _lockCreationLock = new object();

        private readonly CancellationTokenSource _schedulerCancellationToken = new CancellationTokenSource();
        private readonly ActionBlock<SchedulerItem> _scheduler = new ActionBlock<SchedulerItem>(
            action: async (item) => await item.RunAsync(),
            dataflowBlockOptions: new ExecutionDataflowBlockOptions {
                MaxDegreeOfParallelism = ExecutionDataflowBlockOptions.Unbounded,
            }
        );

        public TInstance Get(TId id)
        {
            AssertGetterFunctionHasRunFirstTimeForInstance(id);

            return _instances[id];
        }

        private void AssertGetterFunctionHasRunFirstTimeForInstance(TId id)
        {
            if (_hasRunGetterFunctionFirstTimeForInstance.ContainsKey(id)) {
                return;
            }

            if(!_hasRunGetterFunctionFirstTimeForInstanceLock.ContainsKey(id)){
                lock(_lockCreationLock){
                    if (!_hasRunGetterFunctionFirstTimeForInstanceLock.ContainsKey(id))
                    {
                        _hasRunGetterFunctionFirstTimeForInstanceLock[id] = new object();
                    }
                }
            }

            lock (_hasRunGetterFunctionFirstTimeForInstanceLock[id])
            {
                if (_hasRunGetterFunctionFirstTimeForInstance.ContainsKey(id)) {
                    return;
                }

                RunGetterFunction(id);

                _hasRunGetterFunctionFirstTimeForInstance[id] = true;
            }
        }

        private void RunGetterFunction(TId id)
        {
            var instance = _getterFunction(id);

            _instances.AddOrUpdate(id, instance, (_, __) => instance);

            ScheduleGetterFunction(id);
        }

        private void ScheduleGetterFunction(TId id)
        {
            if (_getterFunctionWaitPeriod == null)
            {
                return;
            }

            _scheduler.Post(new SchedulerItem(this, id));
        }

        protected class SchedulerItem
        {
            public SchedulerItem(IndividualisticCache<TId, TInstance> cache, TId id)
            {
                Cache = cache;
                Id = id;
            }

            private readonly IndividualisticCache<TId, TInstance> Cache;
            private readonly TId Id;

            public async Task RunAsync()
            {
                await Task.Delay(Cache._getterFunctionWaitPeriod.Value, Cache._schedulerCancellationToken.Token).ConfigureAwait(false);

                Cache.RunGetterFunction(Id);
            }
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

You're not afraid of long names which is great. Descriptive long names are definitely better than vague short ones. I think you could probably make the code slightly easier to read though:

getterFunction could be valueFactory
getterFunctionWaitPeriod ccould be refreshInterval or timeToLive
hasRunGetterFunctionFirstTimeForInstanceLock could be initializationLocks

_hasRunGetterFunctionFirstTimeForInstance sounds like a bool or a method that returns a bool. The dictionary is a collection of ids to a value indicating whether it has been initialized but you don't need that. You could just try to get the value out of instances for the given TId and if you don't have one then run the getter function.

I find TId and TInstance a bit odd, TKey and TValue might be more standard.

You're mixing bracing styles here:

 if(!_hasRunGetterFunctionFirstTimeForInstanceLock.ContainsKey(id)){
     lock(_lockCreationLock){
         if (!_hasRunGetterFunctionFirstTimeForInstanceLock.ContainsKey(id))
         {
             _hasRunGetterFunctionFirstTimeForInstanceLock[id] = new object();
         }
     }
}

Either always put { on a new line (preferred) or don't. Mixing and matching is upsetting :(

In general, I find your strategy a bit odd. As I said before, if instances contains your id, it's already been created once so you don't need to track creation separately.

ConcurrentDictionary<TKey, object> initializationLocks = new ConcurrentDictionary<TKey, object>();

public TInstance Get(TKey id)
{
    TValue value;
    if (_instances.TryGetValue(id, out value)
    {
        return value;
    }

    return InitializeId(id);
}

private TValue InitializeId(TKey id)
{
    lock (initializationLocks.AddOrUpdate(id, new object(), (k, v) => v))
    {
        TValue value;
        if (_instances.TryGetValue(id, out value)
        {
            return value;
        }
        RunGetterFunction(id);
        return _instances[id];
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! Regarding the Dictionary of bools - didn't think of that, but then again, it would be good to let the cache have both positives and negatives. It shouldn't re-run the getter function every time someone requests something, just because the getter function happens to return some special value. \$\endgroup\$ – user77309 Jul 8 '15 at 15:01
  • \$\begingroup\$ @BjörnAliGöransson - my suggestion won't rerun the getter function as long as the getter function adds something to the cache... \$\endgroup\$ – RobH Jul 8 '15 at 15:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy