0

In my project I'm using some static variables which I use for storing values during the running lifetime of the application. Now, 99% of the time I'm only reading these values but from time to time I also need to update them and this will happen from different threads.

When thinking about what might happen with two different threads trying to access the same property e.g. concurrent read/write, I started to conclude that some form of synchronization would needed in order to avoid unexpected values being returned between different process or some risk of race conditions.

In essence I needed to derive a single source of truth. I realize that some properties are atomic like booleans, but my methodology mostly applies for the purpose of strings.

One of the challenges is that these static variables are referenced in many places and between different classes, so I also had to figure out an efficient way to solve this challenge without lots of code re-write.

I've decided to use concurrent dictionaries:

public static readonly ConcurrentDictionary<string, string> AppRunTimeStringDictionary = new();
public static readonly ConcurrentDictionary<string, int> AppRunTimeIntegerDictionary = new();
public static readonly ConcurrentDictionary<string, bool> AppRunTimeBooleanDictionary = new();

In my program.cs file, during the earliest stages of startup I simply add all of the properties needed for the running app:

DeviceProvisioning.AppRunTimeBooleanDictionary.TryAdd("UseGpsReceiver", false);
DeviceProvisioning.AppRunTimeStringDictionary.TryAdd("Latitude", String.Empty);
DeviceProvisioning.AppRunTimeStringDictionary.TryAdd("Longitude", String.Empty);

Then in one of my classes I hard code these properties:

public static bool? UseGpsReceiver
{
    get
    {
        if (AppRunTimeBooleanDictionary.TryGetValue("UseGpsReceiver", out var returnedValue))
            return returnedValue;
        return null;
    }
}
public static string? Latitude
{
    get
    {
        if (AppRunTimeStringDictionary.TryGetValue("Latitude", out var returnedValue))
            return returnedValue;
        return null;
    }
}
public static string? Longitude
{
    get
    {
        if (AppRunTimeStringDictionary.TryGetValue("Longitude", out var returnedValue))
            return returnedValue;
        return null;
    }
}

Now for updating these properties, which happens rarely but will be done every now and then, I'm updating these in just one location i.e. using a single method. This way I can use this common method and simply add more prperties to the switch case over time.

public static void SetRunTimeSettings(string property, object value)
{
    switch (property)
    {
        case "UseGpsReceiver":
            // code block
            if (AppRunTimeBooleanDictionary.TryGetValue("UseGpsReceiver", out var useGpsReceiver))
            { AppRunTimeBooleanDictionary.TryUpdate("UseGpsReceiver", (bool)value, useGpsReceiver); }
            break;
        case "Latitude":
            // code block
            if (AppRunTimeStringDictionary.TryGetValue("Latitude", out var latitude))
            { AppRunTimeStringDictionary.TryUpdate("Latitude", (string)value, latitude); }
            break;
        case "Longitude":
            // code block
            if (AppRunTimeStringDictionary.TryGetValue("Latitude", out var longitude))
            { AppRunTimeStringDictionary.TryUpdate("Latitude", (string)value, longitude); }
            break;
    }
}

If I want to update a property then I simply invoke the method as such:

MyClassName.SetRunTimeSettings("UseGpsReceiver", true);
MyClassName.SetRunTimeSettings("Latitude", "51.1234");
MyClassName.SetRunTimeSettings("Longitude", "51.5678");

Because the properties themselves are public static then I can use the getter from anywhere in the app.

From my initial testing, everything seems to work.

Perceived advantages in this approach:

  • Using a separate dictionary for each type of property collection i.e. strings/integers etc, means I can simply add more properties to the dictionary any time in the future without the need for referencing a model class in the dictionary, as opposed to the dictionary below:
public static readonly ConcurrentDictionary<string, myModelClass> AppRunTimeStringDictionary = new();
  • Use of the concurrent dictionary (my understanding) is that any process trying to read the property value from the dictionary will always get the latest value, if a property is being updated then I have less risk in reading an old value. Not such an issue for structured logging but if I was storing keys/secrets/connection strings or anything else, reading an old value might stop some process from being able to function correctly.

  • Using the concurrent dictionary means I don't have to hand craft my own locking mechanisms, which many people seem not to like doing.

  • Dictionary applies its own internal locks on the individual objects, so any property not being updated can still be read by other processes without much delay.

  • If the public static getter ever returned a null value, my thoughts are it would be better to return a null value rather than returning the wrong value. I could always implement some kind of polly or retry mechanism somewhere from the calling process, some short delay before trying to retrieve the property value again (by which time it should have been updated from the other thread that was currently updating it)

Appreciate there will be other ways to approach this, so really what I'm asking here is whether anyone sees any issue in my approach?

I'm not planning to add that many properties to each dictionary, I just want a way to ensure that reads and writes are happening with some form of synchronization and order.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
OJB1
  • 2,245
  • 5
  • 31
  • 63
  • Apart from the boxing of structs in the setter, I can't see any real issues. As a matter of style, I would have preferred explicit `set` proeprties rather than one big monolithic `Set` function, also you can shorten each property to `public static bool? UseGpsReceiver { get => AppRunTimeBooleanDictionary.TryGetValue("UseGpsReceiver", out var returnedValue) ? returnedValue : null; set => AppRunTimeBooleanDictionary.TryUpdate("UseGpsReceiver", value, useGpsReceiver); }`. I probably would have just used `Interlocked.Exchange` with normal fields, but that's just my opinion. – Charlieface Aug 30 '22 at 21:41
  • Is it possible that the `SetRunTimeSettings` can be called concurrently by multiple threads, or the updating is the responsibility of a single thread/worker? – Theodor Zoulias Aug 30 '22 at 23:02
  • does a queue work better instead of a list in your scenario ? – jmvcollaborator Aug 31 '22 at 00:12
  • 1
    Your `SetRunTimeSettings` is awful. It relies on methods that follow the `Try*` pattern, but it itself does not. Also you have a clear bug in the code for the `"Longitude"` case - you're updating `"Latitude"` inside. – Enigmativity Aug 31 '22 at 04:07
  • Also doing a `TryGetValue` just to then be able to call `TryUpdate` is just throwing away all of the value of `Try*` operators anyway. It's a hack. – Enigmativity Aug 31 '22 at 04:09

4 Answers4

1

If you are only setting a single string / int / bool at a time, then you don't need to any thread safety. If you are assigning any single value smaller than a machine word, any reading thread will either see the before value or the after value.

However it looks like you intend to set three values at the same time;

MyClassName.SetRunTimeSettings("UseGpsReceiver", true);
MyClassName.SetRunTimeSettings("Latitude", "51.1234");
MyClassName.SetRunTimeSettings("Longitude", "51.5678");

And I assume you want any reader to see either the old values or the new values. In this case you would need some thread synchronisation around every read / write. Which your current code doesn't have.

You could instead store the three values in a class, then update the reference to that instance in one write operation.

public class GpsSettings{
    public bool UseGpsReceiver { get; init; }
    public double Latitude { get; init; }
    public double Longitude { get; init; }
    public static GpsSettings Current;
}
...

// write
GpsSettings.Current = new GpsSettings {
    UseGpsReceiver = true,
    Latitude = 51.1234,
    Longitude = 51.5678
};

// read
var gps = GpsSettings.Current;
var location = $"{gps.Latitude}, {gps.Longitude}";

// but never do this;
var location = $"{GpsSettings.Current.Latitude}, {GpsSettings.Current.Longitude}";

Jeremy Lakeman
  • 9,515
  • 25
  • 29
  • Hi, The scenario I'm looking for is that if a Thread A starts to update a property, then Thread B tries to read it at the same time, I need Thread B to wait for the property update to complete so that it gets the latest value. Thanks – OJB1 Aug 31 '22 at 08:21
  • If you can reduce contention to a single machine word, aligned in memory. Which all class fields should be. Then memory will either contain the value before the write. Or the value after the write. In other words, there's no time between Thread A "starts" and "complete". In which case, does Thread B ever need to wait for anything? – Jeremy Lakeman Aug 31 '22 at 11:47
  • So I'm suggesting that if you "write" a set of new values, by creating a new instance of a class to represent the set in a new memory location. Then Thread B will either read from the old instance, or the new instance, by copying the reference. Garbage collection will then take care of the rest. If you don't want to allocate memory on every write, then you'll have to synchronize reading and writing in some way to ensure that all readers only ever see a consistent set of values. – Jeremy Lakeman Aug 31 '22 at 11:52
1

Your SetRunTimeSettings is awful. It relies on methods that follow the Try* pattern, but it itself does not. Also doing a TryGetValue just to then be able to call TryUpdate is just throwing away all of the value of Try* operators anyway. It's a hack.

And you have a clear bug in the code for the "Longitude" case - you're updating "Latitude" inside.

I'd suggest going old school and just do this:

private static bool? _UseGpsReceiver;
private readonly static object _UseGpsReceiverLock = new();
public static bool? UseGpsReceiver
{
    get { lock (_UseGpsReceiverLock) return _UseGpsReceiver; }
    set { lock (_UseGpsReceiverLock) _UseGpsReceiver = value; }
}

private static string? _Latitude;
private readonly static object _LatitudeLock = new();
public static string? Latitude
{
    get { lock (_LatitudeLock) return _Latitude; }
    set { lock (_LatitudeLock) _Latitude = value; }
}

private static string? _Longitude;
private readonly static object _LongitudeLock = new();
public static string? Longitude
{
    get { lock (_LongitudeLock) return _Longitude; }
    set { lock (_LongitudeLock) _Longitude = value; }
}

If you don't want to repeat all of the locks then maybe a Locked<T> class might be of use:

public struct Locked<T>
{
    public Locked(T value)
    {
        _value = value;
    }
    private T _value;
    private readonly object _gate = new();
    public T Value
    {
        get { lock (_gate) return _value; }
        set { lock (_gate) _value = value; }
    }
}

Then you can write this:

private static Locked<bool?> _UseGpsReceiver;
public static bool? UseGpsReceiver
{
    get { return _UseGpsReceiver.Value; }
    set { _UseGpsReceiver.Value = value; }
}

private static Locked<string?> _Latitude;
public static string? Latitude
{
    get { return _Latitude.Value; }
    set { _Latitude.Value = value; }
}

private static Locked<string?> _Longitude;
public static string? Longitude
{
    get { return _Longitude.Value; }
    set { _Longitude.Value = value; }
}
Enigmativity
  • 113,464
  • 11
  • 89
  • 172
  • 1
    I think that for most built-in primitive types you could replace the `Locked` with the [`Volatile.Read`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.volatile.read)/[`Volatile.Write`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.volatile.write) methods, and have the same effect (atomicity/visibility) with better performance. – Theodor Zoulias Aug 31 '22 at 05:57
  • Hi, If using the locking mechanisms, would that mean that only a single thread can read them one at at time? My solution requires that multiple threads can read the properties concurrently to avoid a performance penalty in accessing them. Only if a property is being updated do I want all other threads to wait for the result i.e. the new value, thanks – OJB1 Aug 31 '22 at 08:17
  • @OJB1 - What do you think this code does? – Enigmativity Aug 31 '22 at 08:20
  • In your option 1 example, I'm seeing locks on the reads? – OJB1 Aug 31 '22 at 08:24
  • @TheodorZoulias - `Volatile.Read`/`Write` doesn't work on `bool?`. – Enigmativity Aug 31 '22 at 08:25
  • @OJB1 - Yes, you are seeing locks on the reads. But that doesn't answer my question. What do you think this code does? – Enigmativity Aug 31 '22 at 08:26
  • ermm, Thread A starts to update a property, thread B tries to read that property at the same time, but it has to wait until the lock is released from Thread A, so we ensure that Thread B will always get the latest value? – OJB1 Aug 31 '22 at 08:31
  • True, there are no `Volatile` `Read`/`Write` overloads for nullable types (`Nullable`). – Theodor Zoulias Aug 31 '22 at 08:53
  • @OJB1 - "so we ensure that Thread B will always get the latest value" does not follow from the first part of your sentence. – Enigmativity Aug 31 '22 at 08:57
  • 1
    @TheodorZoulias - Bad refactoring. Thanks for the catch on that one. – Enigmativity Aug 31 '22 at 09:05
  • Also you could include a note that the `private static Locked _UseGpsReceiver;` etc fields should **not** be declared `readonly`, because this would result in [copying](https://stackoverflow.com/questions/6063212/does-using-public-readonly-fields-for-immutable-structs-work). – Theodor Zoulias Aug 31 '22 at 09:07
  • @Enigmativity I'm merely asking for your help, please can you explain/talk us through your example. I very rarely use locks so I dont know what the cost/impact would be using them in this scenario. I imagine it's all proportional to the number of threads trying to access the property. – OJB1 Aug 31 '22 at 13:24
  • @OJB1 regarding the performance of the `lock`: *"you can expect to acquire and release a lock in as little as 20 nanoseconds on a 2010-era computer if the lock is uncontended"* ([citation](https://www.albahari.com/threading/part2.aspx#_Locking_Performance)). In other words it can be acquired and released around 50,000,000 times per second (per locker object). It's pretty efficient. – Theodor Zoulias Aug 31 '22 at 20:34
  • Yep I read up on this and in general people are saying up to 50 nanoseconds + any time another thread is waiting, so I'm happy on this basis. I've implemnted Enigmativity's examle option 1 as it was the quickest one for me to retro fit into my code and it seems to be working. Big thanks to you all for your inputs and help :) – OJB1 Aug 31 '22 at 22:39
0

Not everyone would agree with me on this one but my personal approach would be to have a single dictionary of the following type:

Dictionary<string, object>

Wrapped in a separate class with the following methods such as AddValue, GetValue, HasKey, HasValue, and UpdateValue with lock statements. Also notice that you'll have to use somewhat generic methods in order to be able to retrieve the value with the actual type and a default value. For example:

public static T GetValue<T>(string key, T defaultValue)

Also, I don't see a problem with your approach but if you want to synchronize things then you'll need n dedicated locks for n dictionaries which I don't think is a clean way; unless I'm missing something, and of course registering multiple dictionaries in design time can be a headache.

rizistt
  • 38
  • 1
  • 4
0

Alternatively to using multiple ConcurrentDictionary<string, T> collections, or a single ConcurrentDictionary<string, object>, or the Locked<T> struct shown in Enigmativity's answer, you could just store the values in immutable and recyclable Tuple<T> instances, and store these in private volatile fields:

private static volatile Tuple<bool?> _UseGpsReceiver;
public static bool? UseGpsReceiver
{
    get { return _UseGpsReceiver?.Item1; }
    set { _UseGpsReceiver = new(value); }
}

private static volatile Tuple<string> _Latitude;
public static string Latitude
{
    get { return _Latitude?.Item1; }
    set { _Latitude = new(value); }
}

private static volatile Tuple<string> _Longitude;
public static string Longitude
{
    get { return _Longitude?.Item1; }
    set { _Longitude = new(value); }
}

Pros: Both the reading and the writing are lock-free. An unlimited number of readers and writers can read and update the values at the same time, without contention.

Cons: Every time a value is updated, a new Tuple<T> is instantiated, adding pressure on the .NET garbage collector. This reduces the appeal of this approach in case the values are updated too frequently. Also if you have dozens of properties like these, it might be easy to introduce subtle bugs by omitting the important volatile keyword by mistake.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104