Correct way to convert method to async in C #?

Question

I am trying to convert the following method (simplified example) to asynchronous, since calling cacheMissResolver can be costly in terms of time (database search, 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 is a lot of material on the Internet about consuming asynchronous methods, but the tips I found when creating them seem less clear. Given that this is intended to be part of the library, are 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; } } 

SemaphoreSlim use the correct way to replace the lock statement with the async method? (I cannot wait for the lock statement in the body.)

Should I make a cacheMissResolver argument of type Func<Task<Thing>> ? Although it is the responsibility of the resolver async function to Task.Run call (wrapping in Task.Run , I know that it will be unloaded into the background thread if it takes a long time).

Thanks.

Answer 1

Does SemaphoreSlim use the correct way to replace the lock statement with the async method?

Yes.

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

Yes. This will allow the caller to provide a default asynchronous operation (such as IO), instead of making it only suitable for a job that has been running for a long time. (Despite the fact that it still supports CPU usage, just using the Task.Run caller if that's what they want to do.)

---

Other than that, just note that you don't need to specify await Task.FromResult(...); Wrapping a value in a Task to immediately expand it is pointless. Just use the result directly in such situations, in this case return the cached value directly. What you are doing is actually wrong, just unnecessarily complicating / confusing the code.

Answer 2

If your cache is in memory (it looks like it is), then consider task caching, not results. This has a good side property, if two methods request the same key, only one permit request is executed. Also, since only the cache is locked (and not allowing operations), you can continue to use simple locking.

 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 result in caching of exceptions that you might not need. One way to avoid this is to allow the exception task to enter the cache initially, but then trim it with the following request:

 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 that this extra check is not needed if you have another process that periodically truncates the cache.