Thread safe cache to sql call in c#

Question

I'm unsure whether the following is thread safe. I want to say it is because the static state is only assigned to within the lock. However I'm more of a js programmer so this is out of my comfort zone and would appreciate a more seasoned pair of eyes.

The purpose is simple - this class intends to cache application settings (e.g. help desk phone number) from the database so we're querying the database a maximum once every 3 minutes.

If there's a library or builtin class intended for this then please let me know. I googled for a while and struggled to find pertinent info.

public static class AppSettings { private static readonly object stateLock = new object(); private static TaskCompletionSource<AppSettingsModel> Cached { get; set; } private static DateTime? LastFetched { get; set; } public static async Task<AppSettingsModel> Get() { TaskCompletionSource<AppSettingsModel> prevCache; bool returnCache = false; lock (stateLock) { prevCache = Cached; if (LastFetched.HasValue) { if (DateTime.Now.Subtract(LastFetched.Value).TotalMinutes >= 3) Cached = null; else if (Cached != null) returnCache = true; } if (!returnCache) { Cached = new TaskCompletionSource<AppSettingsModel>(); LastFetched = DateTime.Now; } } if (returnCache) return await Cached.Task; try { Cached.SetResult(await GetAppSettingsFromDB()); } catch (Exception ex) { Console.Error.WriteLine(ex.ToString()); if (prevCache != null) Cached.SetResult(await prevCache.Task); else Cached.SetResult(new AppSettingsModel()); } return await Cached.Task; } } 

Answer 1

Naming

In your provided code sample, you are using inconsistent naming. I would suggest trying to pursue consistency to make maintenance easier. For C# you can find several guidelines what you can follow (or you can align with your company standards):

• Identifier Names
• Coding Conventions
• etc.

TaskCompletionSource

This utility class is not meant to be used as a cache. You should consider this as a producer side helper structure to wrap your synchronous/asynchronous logic into a Task which could be awaited by the consumer.

You do not even take advantage of the TCS at all, it just makes your code more bloated. You could simply define your Cached field as AppSettingsModel.

• Then all these Cached.SetResult(...) calls could be replaced with simple assignment expressions Cache = ....
• Also the return await Cached.Task; could be rewritten to return Cached;

MemoryCache

Gladly you don't need to reinvent the wheel. There are many cache implementations like the System.Runtime.Caching.MemoryCache or the Microsoft.Extensions.Caching.Memory.MemoryCache.

System.Runtime.Caching.MemoryCache

It has a CacheItemPolicy concept which could be used to define retention/update rules. For your use case this could be as simple as this:

var policy = new CacheItemPolicy(); policy.AbsoluteExpiration = DateTimeOffset.Now.AddMinutes(3.0); 

With this policy your code becomes simpler since you don't have to write logic to decide is this cached item considered fresh or stale. It is done on your behalf by the MemoryCache.

Microsoft.Extensions.Caching.Memory.MemoryCache

It has several GetOrCreateAsync overloads to support many use cases. This method either tries to retrieve data from cache or tries to create a new entry if data is not present.

 var cachedValue = await _memoryCache.GetOrCreateAsync( "AppSettingsModel", cacheEntry => { cacheEntry.AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(3); return await GetAppSettingsFromDB(); }); 

Answer 2

After learning about alternatives to MemoryCache which support GetOrSetAsync atomicity, here's the code I'm going with using FusionCache. It should be functionally the same as my original implementation intended.

public static class AppSettings { private static readonly FusionCache Cache = new FusionCache(GetCacheOptions()); public static ValueTask<AppSettingsModel> Get() { return Cache.GetOrSetAsync( // the cache holds key-value pairs but we only need it for a single entry key: "app settings", factory: _ => GetAppSettingsFromDB(), // in the case an error occurs and we don't have a previous entry cached // (i.e. the first call fails) then return an empty model. failSafeDefaultValue: new AppSettingsModel() ); } // // helper methods // private static FusionCacheEntryOptions GetCacheEntryOptions() { return new FusionCacheEntryOptions() { // Cache entries for 3 minutes before attempting a database query to // refresh the value Duration = TimeSpan.FromMinutes(3), // If an error occurs when fetching from the database then handle gracefully IsFailSafeEnabled = true, // If an error occurs then return the previously cached entry (forever) FailSafeMaxDuration = TimeSpan.MaxValue, // If an error occurs then don't hit the database for another three minutes FailSafeThrottleDuration = TimeSpan.FromMinutes(3), }; } private static FusionCacheOptions GetCacheOptions() { return new FusionCacheOptions() { DefaultEntryOptions = GetCacheEntryOptions() }; } } 

As for my original question about my implementation being thread-safe, my intent was to ask about thread safety in the sense that the database call would only be made a maximum once every three minutes. From my reading, apparently "thread-safety" means something different since MemoryCache's GetOrCreateAsync is considered thread-safe yet lacks atomicity. I'm not the only one who finds this distinction disingenuous because if we're caching something then the operation is likely expensive and shouldn't be ran more than necessary. But, it's unimportant since FusionCache solves the problem.