When new value of volatile variable, that was set inside lock section, will available to other threads?

Question

I have method Test, that can be executed from many threads, and I don't want to lock all threads, who want execute them, while method already executing. My idea is create volatile isProcessing and set it to true, while execution is active.

But I don't know the C# memory model, and can't understand when other threads will see new value of isProcessing. Will it do instantly after setting isProcessing value or only after quit from lock section?

Code sample:

private volatile bool isProcessing = false;

private void Test()
{
  if (isProcessing) return;   
  lock(this){
    try{   
      isProcessing = true;
      //do something
    }
    finally{ 
      isProcessing = false;  
    }
  }
}

It is not correct code, the `if (isProcessing)` test needs to be inside the lock {} as well. Which ensures that the implicit memory barrier inside the lock implementation updates it. Delete *volatile* to make it sane. A low-lock approach is Interlocked.CompareExchange(). You'd have to hit this code hundreds of thousands of times per second to make it pay off. — Hans Passant, Jun 19 '18 at 11:38
@HansPassant, if i will move 'isProcessing' checking inside lock section, all threads will be locked. My goal is not do that. — Frank59, Jun 19 '18 at 11:41
Write correct code first, optimize only when you discover that it is necessary. — Hans Passant, Jun 19 '18 at 11:42
@mjwills, instead //do something should be some work with RabbitMq connection and http-request to external systems. I think, i can't post this code here, but it's kind of work that i don't want to do too often and in parallel. — Frank59, Jun 19 '18 at 11:49
Have you considered some kind of queue (e.g. `BlockingCollection`) instead? — mjwills, Jun 19 '18 at 11:51
@mjwills, i didn't think about queues or other sync structures for that problem. Right now i want to understand, how works memory barires for volatile inside lock section. If it's work, how i expect, so it would be enough — Frank59, Jun 19 '18 at 11:57
@HansPassant: The code is actually **correct**: as far as I understand, C# standard doesn't forbid access to volatile variable concurrently from different threads. One may tread `if` outside of critical section as *fast-path*: If it is easy to determine, that processing is performed now, then do nothing. Of course, it is still possible to wait on `lock()` in case of `isProcessing` found to be *false*. — Tsyvarev, Jun 19 '18 at 11:58
Do yourself a favor and use `lock` (without `volatile`), even just for reading the value, unless you can *prove* you've got a bottleneck. Read questions like [this](https://stackoverflow.com/q/3493931/4137916), and all the answers and comments, to get clarity on how even experts get confused by this stuff. You are about ten times more likely to introduce an obscure bug when your code is run on something that's not an x86 then you are likely to get some sort of measurable performance gain. Threaded code is absolutely not the place for optimization up front. — Jeroen Mostert, Jun 19 '18 at 12:23
So i think, i will try to write code use lock only, without fast path outside of lock section. But if anyone will write answer with explanation, how will work my code sample, I'll be happy to vote up this answer. — Frank59, Jun 19 '18 at 12:31
Any time a program keeps a lock locked for longer than it takes to assign a few variables, that is a strong [code smell](https://en.wikipedia.org/wiki/Code_smell). There probably is a better way to solve your problem, but nobody can help you with it if they don't know what the problem is: What does `//do something` actually _do?_ — Solomon Slow, Jun 19 '18 at 14:59

score 1 · Accepted Answer · answered Aug 25 '18 at 22:39

Here's how to do this with two separate locks. Note it's not good practice to lock objects that are possibly visible to other call sites.

    private bool isProcessing = false;
    private object readLock = new object();
    private object processingLock = new object();
    private void Test()
    {
        Monitor.Enter(readLock);
        if (isProcessing)
        {
            Monitor.Exit(readLock);
            return;
        }
        lock (processingLock)
        {
            isProcessing = true;
            Monitor.Exit(readLock);
            try
            {
                //do something
            }
            finally
            {
                isProcessing = false;
            }
        }
    }

A simpler solution is just to use a Mutex instead of a using Monitors.

    Mutex processingMutex = new Mutex(false);
    private void Test2()
    {
        if (!processingMutex.WaitOne(0))
        {
            return;
        }
        try
        {
            //do processing
        }
        finally
        {
            processingMutex.ReleaseMutex();
        }
    }

The Mutex will also enable you to wait briefly to see if the other thread finishes by setting the argument to WaitOne to a non-zero value.

When new value of volatile variable, that was set inside lock section, will available to other threads?

1 Answers1