CODEWITHSUNDEEP
c#collectionsthread-safety
JOBG
JOBG

Reputation: 4624

What options do i have to make this code thread safe?

I have this segment of code , a lot of things skipped for brevity but the scene is this one:

 public class Billing
    {
        private List<PrecalculateValue> Values = new List<PrecalculateValue>();

        public int GetValue(DateTime date)
        {
            var preCalculated = Values.SingleOrDefault(g => g.date == date).value;
            //if exist in Values, return it
            if(preCalculated != null)
            {
               return preCalculated;
            }

            // if it does not exist calculate it and store it in Values
            int value = GetValueFor(date);
            Values.Add(new PrecalculateValue{date = date, value = value});

            return value;
        }

        private object GetValueFor(DateTime date)
        {
            //some logic here
        }
    }

I have a List<PrecalculateValue> Values where i store all the values i already calculated for later use, i do these mainly because i don't want to recalculate things twice for the same client, each calculation involve a lot of operations and take between 500 and 1000 ms, and there is a big chance of reuse that value, because of some recursion involved in the hole billing class.

All of these work perfectly until i made a test where i hit two simultaneous calculations for two different clients, and the line Values.Single(g => g.date == date).value returned an exception because it found more than one result in the collection. So i checked the list and it stored values of both clients in the same list. What can i do to avoid this little problem?

Upvotes: 1

Views: 178

Answers (2)

Dan Tao
Dan Tao

Reputation: 128317

Well, first of all, this line:

return Values.Single(g => g.date == date).value;

makes it so that the subsequent lines will never be called. I'm guessing you've paraphrased your code a little bit here?

If you want to synchronize writes to your Values list, the most straightforward way would be to lock on a common object everywhere in the code that you're modifying the list:

int value = GetValueFor(date);

lock (dedicatedLockObject) {
    Values.Add(new PrecalculateValue{date = date, value = value});
}

return value;

But here's something else worth noting: since it looks like you want to have one PrecalculateValue per DateTime, a more appropriate data structure would probably be a Dictionary<DateTime, PrecalculateValue> -- it will provide lightning-fast, O(1) lookup based on your DateTime key, as compared to a List<PrecalculateValue> which would have to iterate to find what you're looking for.

With this change in place, your code might look something like this:

public class Billing
{
    private Dictionary<DateTime, PrecalculateValue> Values = 
        new Dictionary<DateTime, PrecalculateValue>();

    private readonly commonLockObject = new object();

    public int GetValue(DateTime date)
    {
        PrecalculateValue cachedCalculation;

        // Note: for true thread safety, you need to lock reads as well as
        // writes, to ensure that a write happening concurrently with a read
        // does not corrupt state.
        lock (commonLockObject) {
            if (Values.TryGetValue(date, out cachedCalculation))
                return cachedCalculation.value;
        }

        int value = GetValueFor(date);

        // Here we need to check if the key exists again, just in case another
        // thread added an item since we last checked.
        // Also be sure to lock ANYWHERE ELSE you're manipulating
        // or reading from the collection.
        lock (commonLockObject) {
            if (!Values.ContainsKey(date))
                Values[date] = new PrecalculateValue{date = date, value = value};
        }

        return value;
    }

    private object GetValueFor(DateTime date)
    {
        //some logic here
    }
}

And one last piece of advice: unless it's critical that no more than one of a particular value exist in your collection, the Single method is overkill. If you'd rather just get the first value and disregard potential duplicates, First is both safer (as in, less chance of an exception) and faster (because it doesn't have to iterate over the entire collection).

Upvotes: 5

Paul Creasey
Paul Creasey

Reputation: 28824

Could use something like this

public int GetValue(DateTime date)
{

    var result = Values.Single(g => g.date == date) ?? GetValueFor(date);

    lock (Values)
    {
        if (!Values.Contains(result)) Values.Add(result);
    }
    return result.value;
}

private PrecalculateValue GetValueFor(DateTime date)
{
    //logic
    return new PrecalculateValue() ;
}

Would advise using a dictionary though for a list of key value pairs.

Upvotes: 1

Related Questions