6
\$\begingroup\$

I need simple caching for one of my web-services so I use the Microsoft.Extensions.Caching.Memory package. I wrapped it with SemaphoreSlim to make sure reading and writing are safe. I'm not sure what I'm doing here is entirely correct. What do you think? Is this wrapper OK or can I turn GetOrCreateAsync into an extension for IMemoryCache and it'll handle everything?

public class CacheService
{
    private static readonly SemaphoreSlim Locker = new SemaphoreSlim(1, 1);

    private readonly IMemoryCache _cache;

    public CacheService(IMemoryCache cache)
    {
        _cache = cache;
    }

    public async Task<T> GetOrCreateAsync<T>(object key, Func<Task<T>> create)
    {
        await Locker.WaitAsync();
        try
        {
            if (_cache.TryGetValue(key, out var entry))
            {
                return (T)entry;
            }
            else
            {
                return _cache.Set(key, await create(), new MemoryCacheEntryOptions
                {
                    AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(17)
                });
            }
        }
        finally
        {
            Locker.Release();
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

Sorry this is a bit of a ramble, but a few things jump out at me:

  1. 17?!?! I don't have to tell you about magic numbers! This should probably be configurable... somehow...

  2. Why is Locker static? It doesn't make sense to restrict access to one cache because a totally different cache is currently in use.

  3. Locking the cache while you create an item isn't necessarily going to end well: it would be all too easy for create to try to access the cache, and now you have a deadlock. The 'easiest' way to resolve this would be to slightly abuse the cache, and instead of storing the values themselves store some wrapper which indicates whether the value is written yet with a different concurrency mechanism. This way, you can add a 'pending' value to the cache immediately, which can be initialised outside the lock.

    This increases the complexity of dealing with create failures, because you have to communicate between threads with some other object, but now you can only dead-lock yourself by trying to access the object your are currently creating. It would be nice to detect this, but I think it might be impossible, so you probably want to give whatever mechanism awaits the pending entry a time-out. I don't think you can do better than this without passing the complexity off to the caller, which of course would ruin the nice API.

    A better solution would probably be to provide a completely separate concurrency mechanism for handling 'pending' values, so that there is less overhead and complexity accessing the cache (presumably the more common operation). It wouldn't store the values, but rather just provide a mechanism to wait for them to appear in the proper cache (on a key-by-key basis).

  4. I'd prefer that the cast to (T) was checked first, so that you can throw a highly-specific exception explaining that whatever was in the cache was not what the caller was expecting.

  5. Since you are providing the access control, your class is probably also responsible for disposing the cache: it should provide this facility. Disposing Locker is also a concern. This adds to the opportunity for a consumer to get cryptic error messages if they are trying to access the cache as it is disposed.

  6. Public APIs should have inline documentation (///), so that the maintainer knows what it's meant to do, and consumers can find out how to use them correctly without consulting the source-code.

\$\endgroup\$
  • \$\begingroup\$ 1) haha, the magic number is really magic. I picked it because I like primes, it's not configurable; I just thought it's nither to long nor too short :-P 2) oh, I sometimes make too many assumptions; I register this with Autofac as SingleInstance but you're right, making it static was pretty stupid of me 3) this is pure rocket-sciene ;-o I really had hoped that this library could do better than just be a dictionary with a timer :-\ 4) oops, need to improve that; 5) oops, I didn't notice it was disposable. \$\endgroup\$ – t3chb0t Jul 30 at 13:08
  • \$\begingroup\$ Could there really be a deadlock? The Locker allows only a single access at a time, doesn't it? \$\endgroup\$ – t3chb0t Jul 30 at 13:09
  • 3
    \$\begingroup\$ @t3chb0t I don't see why there couldn't be a deadlock (but maybe I'm confused, I haven't got much sleep lately!): if create calls GetOrCreateAsync, then he's not going to get through. \$\endgroup\$ – VisualMelon Jul 30 at 13:13
  • 1
    \$\begingroup\$ crappy-crapp, you're absolutely right! o_O I feel so dumb :-] \$\endgroup\$ – t3chb0t Jul 30 at 13:14
6
\$\begingroup\$

You can simplify GetOrCreateAsync() a bit, because IMemoryCache has an extension called Microsoft.Extensions.Caching.Memory.GetOrCreateAsync():

public async Task<T> GetOrCreateAsync<T>(object key, Func<ICacheEntry, Task<T>> create)
{
  await Locker.WaitAsync();
  try
  {
    return await _cache.GetOrCreateAsync(key, create);
  }
  finally
  {
    Locker.Release();
  }
}

or:

public async Task<T> GetOrCreateAsync<T>(object key, Func<Task<T>> create)
{
  await Locker.WaitAsync();
  try
  {
    return await _cache.GetOrCreateAsync(key, ice => {
      ice.AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(17);
      return create();
    });
  }
  finally
  {
    Locker.Release();
  }
}

if you want to control the expiration as in your original.

\$\endgroup\$
  • 5
    \$\begingroup\$ oh boy, me dumb**3 I probably should get some sleep ;-] \$\endgroup\$ – t3chb0t Jul 30 at 13:49

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.