How to make a property in a class thread safe

Question

I have a console application which is having some functionality and I have a class doing some operation like publishing some message to a service. Now on each message publish on that class I am incrementing a counter.And I have a delegate associated to that counter, so that after a limit is reached I have a event on the Program.cs to block the call.Now I am doing the publish part async. So how to protect the counter property.

Below is my code:

Program.cs

var objMessageManager = new MessageManager();
objMessageManager.MaximumLimitToStopRequest += objMessageManager_MaximumLimitToStopRequest;

static void objMessageManager_MaximumLimitToStopRequest(object sender, int ItemCount)
{
    if (ItemCount > Settings.MessageLimit)
    {
        Task.Delay(Settings.MessageDelayTime);
    }
}

And below is the code for MessageManager.cs class

internal delegate void StopRequestEventHandler(object sender, int ProcessedItem);

public event StopRequestEventHandler MaximumLimitToStopRequest;

private int ProcessedItem;

private int noOfProcessed;

public int NoOfProcessed
{
    get
    {
        return ProcessedItem;
    }
    private set 
    {
        ProcessedItem = value;
        if (MaximumLimitToStopRequest != null)
        {
            MaximumLimitToStopRequest(this, ProcessedItem);
        }
    }
}

And This is the code inside the method

internal async void CreateMessage(xyz sMessageObj)
{
    try
    {
        await Task.Run(() =>
        {
            // Code to publish the message
        });

        Interlocked.Increment(ref noOfProcessed);
        NoOfProcessed = noOfProcessed;
    }
    catch (Exception ex)
    {
        Log.Error("Exception occurred .", ex);
    }
}

Please ignore naming mistake for variable and all. The application is running fine. I need help to make it thread safe for both reading and writing/incrementing NoOfProcessed property.

Can you apply any of the advice at http://stackoverflow.com/q/7161413/16391 to your problem? — StingyJack, Aug 21 '15 at 15:01
Have you considered using `Interlocked.Exchange()` for Read and write of backing field of the property? — vendettamit, Aug 21 '15 at 15:24
@leppie I think that this question is relevant to v4.0 as there is `Task` and `async/await`. This isn't general question about multithreading in .net, in my opinion. — VMAtm, Aug 25 '15 at 14:46

VMAtm · Accepted Answer · 2015-08-27T11:35:08.577

No, your code isn't thread-safe in any case. First of all, in StopRequestEventHandler class you didn't mark a ProcessedItem or noOfProcessed as volatile. Second, Interlocked.Increment isn't enough, you have to use the CAS-operation of Interlocked, CompareExchange. Third, you should consider to create a queue as the ProcessedItem property can be a target for the race-condition.

A queue in easiest form is a simply Array of updated values, so we have an array, and currentPosition on it. After CAS-Exchange operation you simply use the array item based on index independently from other threads. So code will be like this (this is a very straight-forward implementation, you should try to write your own code):

int[] processedItems = new int[INITIAL_SIZE];
int currentItem = 0;

var current = currentItem;
// make first attempt...
if (Interlocked.CompareExchange(ref currentItem, current + 1, current) != current)
{
    // if we fail, go into a spin wait, spin, and try again until succeed
    var spinner = new SpinWait();
    do
    {
        spinner.SpinOnce();
        current = currentItem;
    }
    while (Interlocked.CompareExchange(ref currentItem, current + 1, current) != current);
}
// store the value in array
processedItems[current] = value

thanks for your reply. Can you please give an example or reference to a link about the queue you mentioned on the answer. — choudhury smrutiranjan parida, Aug 23 '15 at 01:57

kha · Answer 2 · 2015-08-21T15:42:50.457

I'm very confused with your code to be honest.

You have

  private int ProcessedItem;
  private int noOfProcessed;

And then your property (which is also weirdly named) is

public int NoOfProcessed
{
    get
    {
        return ProcessedItem;
    }
    private set
    {
        ProcessedItem = value;
        if (MaximumLimitToStopRequest != null)
        {
            MaximumLimitToStopRequest(this, ProcessedItem);
        }
    }
}

but then you are incrementing noOfProcessed in your logic (Interlocked.Increment(ref noOfProcessed);

I suggest rewriting your code simply like this:

private int noOfProcessed;
...
...
    Interlocked.Increment(ref noOfProcessed);
    if (MaximumLimitToStopRequest != null) // Warning: this is NOT thread safe
    {
        MaximumLimitToStopRequest(this, noOfProcessed);
    }
...
public int NoOfProcessed
        {
            get
            {
                return noOfProcessed;
            }
        }

Or like this (which will be slower but safer since you're wrapping the event invocation in a thread-safe manner as well)

lock(_syncLock)
{
    ++noOfProcessed;
    if (MaximumLimitToStopRequest != null)
    {
        MaximumLimitToStopRequest(this, noOfProcessed);
    }
}

I do quite like the queue idea by VMAtm though. A thread-safe queue seems to be better suited to your task but the above solution (specially the lock one) should be thread-safe as long as you're operating in a single app domain and that's the only place the event is invoked.

score -1 · Answer 3 · answered Aug 25 '15 at 12:44

You could use a ReaderWriterLockSlim.

It's a lock that will permits you to be thread safe and to maintains a bits of performance.

use

try
{
  // lock you code in write to change the value and read to just read it.
}
finally
{
 // release the lock
}

The lock (ReaderWriterLockSlim) let reader pass and lock only when the write lock is ask.

How to make a property in a class thread safe

Program.cs

And below is the code for MessageManager.cs class

And This is the code inside the method

3 Answers3