Correct way to convert method to async in C#?

Question

I'm attempting to convert the following method (simplified example) to be asynchronous, as the cacheMissResolver call may be expensive in terms of time (database lookup, network call):

// Synchronous version
public class ThingCache
{
    private static readonly object _lockObj;
    // ... other stuff

    public Thing Get(string key, Func<Thing> cacheMissResolver)
    {
        if (cache.Contains(key))
            return cache[key];

        Thing item;

        lock(_lockObj)
        {
            if (cache.Contains(key))
                return cache[key];

            item = cacheMissResolver();    
            cache.Add(key, item);
        }

        return item;
    }
}

There are plenty of materials on-line about consuming async methods, but the advice I have found on producing them seems less clear-cut. Given that this is intended to be part of a library, are either of my attempts below correct?

// Asynchronous attempts
public class ThingCache
{
    private static readonly SemaphoreSlim _lockObj = new SemaphoreSlim(1);
    // ... other stuff

    // attempt #1
    public async Task<Thing> Get(string key, Func<Thing> cacheMissResolver)
    {
        if (cache.Contains(key))
            return await Task.FromResult(cache[key]);

        Thing item;

        await _lockObj.WaitAsync();

        try
        {
            if (cache.Contains(key))
                return await Task.FromResult(cache[key]);

            item = await Task.Run(cacheMissResolver).ConfigureAwait(false);
            _cache.Add(key, item);
        }
        finally
        {
            _lockObj.Release();
        }

        return item;
    }

    // attempt #2
    public async Task<Thing> Get(string key, Func<Task<Thing>> cacheMissResolver)
    {
        if (cache.Contains(key))
            return await Task.FromResult(cache[key]);

        Thing item;

        await _lockObj.WaitAsync();

        try
        {
            if (cache.Contains(key))
                return await Task.FromResult(cache[key]);

            item = await cacheMissResolver().ConfigureAwait(false);
            _cache.Add(key, item);
        }
        finally
        {
            _lockObj.Release();
        }

        return item;
    }
}

Is using SemaphoreSlim the correct way to replace a lock statement in an async method? (I can't await in the body of a lock statement.)

Should I make the cacheMissResolver argument of type Func<Task<Thing>> instead? Although this puts the burden of making sure the resolver func is async on the caller (wrapping in Task.Run, I know it will be offloaded to a background thread if it takes a long time).

Thanks.

Answer 1

If your cache is in-memory (it looks like it is), then consider caching the tasks rather than the results. This has a nice side property if two methods request the same key, only a single resolving request is made. Also, since only the cache is locked (and not the resolving operations), you can continue using a simple lock.

public class ThingCache
{
  private static readonly object _lockObj;

  public async Task<Thing> GetAsync(string key, Func<Task<Thing>> cacheMissResolver)
  {
    lock (_lockObj)
    {
      if (cache.Contains(key))
        return cache[key];
      var task = cacheMissResolver();
      _cache.Add(key, task);
    }
  }
}

However, this will also cache exceptions, which you may not want. One way to avoid this is to permit the exception task to enter the cache initially, but then prune it when the next request is made:

public class ThingCache
{
  private static readonly object _lockObj;

  public async Task<Thing> GetAsync(string key, Func<Task<Thing>> cacheMissResolver)
  {
    lock (_lockObj)
    {
      if (cache.Contains(key))
      {
        if (cache[key].Status == TaskStatus.RanToCompletion)
          return cache[key];
        cache.Remove(key);
      }
      var task = cacheMissResolver();
      _cache.Add(key, task);
    }
  }
}

You may decide this extra check is unnecessary if you have another process pruning the cache periodically.

Answer 2

Is using SemaphoreSlim the correct way to replace a lock statement in an async method?

Yes.

Should I make the cacheMissResolver argument of type Func<Task<Thing>> instead?

Yes. It'll allow the caller to provide an inherently asynchronous operation (such as IO) rather than making this only suitable for work that is long running CPU bound work. (While still supporting CPU bound work by simply having the caller use Task.Run themselves, if that's what they want to do.)

---

Other than that, just note that there's not point in having await Task.FromResult(...); Wrapping a value in a Task just to immediately unwrap it is pointless. Just use the result directly in such situations, in this case, return the cached value directly. What you're doing isn't really wrong, it's just needlessly complicating/confusing the code.