C# manual lock/unlock

Question

I have a function in C# that can be called multiple times from multiple threads and I want it to be done only once so I thought about this:

class MyClass
{
    bool done = false;
    public void DoSomething()
    {
        lock(this)
            if(!done)
            {
                done = true;
                _DoSomething();
            }
    }
}

The problem is _DoSomething takes a long time and I don't want many threads to wait on it when they can just see that done is true.
Something like this can be a workaround:

class MyClass
{
    bool done = false;
    public void DoSomething()
    {
        bool doIt = false;
        lock(this)
            if(!done)
                doIt = done = true;
        if(doIt)
             _DoSomething();
    }
}

But just doing the locking and unlocking manually will be much better.
How can I manually lock and unlock just like the lock(object) does? I need it to use same interface as lock so that this manual way and lock will block each other (for more complex cases).

Rentfree

Can I ask what _DoSomething() does? – Sash

Consider the carefully the disavantages of using lock(this): Why is lock(this) {...} bad? – Loggins

I am confused. If multiple threads are waiting a long time on _DoSomething, isn't that what you want? If two threads both say "synchronously ensure that _DoSomething has been called" at the same time then they both have to wait until at least one of them does it, no? Why is this a problem? Waiting until its done sounds like the desired feature. Can you explain? – Mendel

Also, can you explain why you believe that locking and unlocking explicitly with the monitors is better than the lock statement? – Mendel

I don't want other threads to wait until its done, I want other threads make sure its initiated. – Rentfree

@Dani: What are the other threads supposed to do if it isn't done, continue executing as though it is? That's broken, because then it isn't initialized and they're executing as though it were. – Indrawn

@Greg: It depends on what "DoSomething" actually does. For example, suppose "DoSomething" is "send an email to the administrator when the service starts up". That's something that you only want done once. If ten threads all try to send email at the same time, nine of them can immediately return if the tenth is currently working on the problem; the assumption here is that "working on it" is enough; you don't have to wait around for it to finish. Since the original poster has not told us what sort of thing DoSomething is doing, it is really hard to give a correct analysis of the code. – Mendel

Answer 1

The lock keyword is just syntactic sugar for Monitor.Enter and Monitor.Exit:

Monitor.Enter(o);
try
{
    //put your code here
}
finally
{
    Monitor.Exit(o);
}

is the same as

lock(o)
{
    //put your code here
}

Archaeornis

Note that in C# 4 we changed the codegen slightly so that the enter goes inside the try block. See blogs.msdn.com/b/ericlippert/archive/2009/03/06/… for the messy details. – Mendel

@EricLippert Thanks. I just adapted the answer. – Archaeornis

The edit makes the code incorrect. Suppose a thread abort exception is thrown after the try is entered but before the lock is taken. The finally block will run and release a lock that was never taken. – Mendel

Answer 2

Thomas suggests double-checked locking in his answer. This is problematic. First off, you should not use low-lock techniques unless you have demonstrated that you have a real performance problem that is solved by the low-lock technique. Low-lock techniques are insanely difficult to get right.

Second, it is problematic because we don't know what "_DoSomething" does or what consequences of its actions we are going to rely on.

Third, as I pointed out in a comment above, it seems crazy to return that the _DoSomething is "done" when another thread is in fact still in the process of doing it. I don't understand why you have that requirement, and I'm going to assume that it is a mistake. The problems with this pattern still exist even if we set "done" after "_DoSomething" does its thing.

Consider the following:

class MyClass 
{
     readonly object locker = new object();
     bool done = false;     
     public void DoSomething()     
     {         
        if (!done)
        {
            lock(locker)
            {
                if(!done)
                {
                    ReallyDoSomething();
                    done = true;
                }
            }
        }
    }

    int x;
    void ReallyDoSomething()
    {
        x = 123;
    }

    void DoIt()
    {
        DoSomething();
        int y = x;
        Debug.Assert(y == 123); // Can this fire?
    }

Is this threadsafe in all possible implementations of C#? I don't think it is. Remember, non-volatile reads may be moved around in time by the processor cache. The C# language guarantees that volatile reads are consistently ordered with respect to critical execution points like locks, and it guarantees that non-volatile reads are consistent within a single thread of execution, but it does not guarantee that non-volatile reads are consistent in any way across threads of execution.

Let's look at an example.

Suppose there are two threads, Alpha and Bravo. Both call DoIt on a fresh instance of MyClass. What happens?

On thread Bravo, the processor cache happens to do a (non-volatile!) fetch of the memory location for x, which contains zero. "done" happens to be on a different page of memory which is not fetched into the cache quite yet.

On thread Alpha at the "same time" on a different processor DoIt calls DoSomething. Thread Alpha now runs everything in there. When thread Alpha is done its work, done is true and x is 123 on Alpha's processor. Thread Alpha's processor flushes those facts back out to main memory.

Thread bravo now runs DoSomething. It reads the page of main memory containing "done" into the processor cache and sees that it is true.

So now "done" is true, but "x" is still zero in the processor cache for thread Bravo. Thread Bravo is not required to invalidate the portion of the cache that contains "x" being zero because on thread Bravo neither the read of "done" nor the read of "x" were volatile reads.

The proposed version of double-checked locking is not actually double-checked locking at all. When you change the double-checked locking pattern you need to start over again from scratch and re-analyze everything.

The way to make this version of the pattern correct is to make at least the first read of "done" into a volatile read. Then the read of "x" will not be permitted to move "ahead" of the volatile read to "done".

Mendel

Eric, how do you suggest to perform the volatile read then? Using Thread.VolatileRead? – Necropsy

@Thomas: I suggest never using double checked locking unless your name is Joe Duffy. If you do want to live dangerously and invent your own versions of double-checked locking then you probably don't want to use Thread.VolatileRead since it can have bad performance characteristics -- in some implementations I believe it creates a full memory fence, though I might be misremembering. You probably want to simply make "done" volatile, even though that makes some of its reads and writes unnecessarily volatile. – Mendel

Your volatile and memory sharing explanation helped clear some other bugs I had, but the other threads don't need to use any information generated by ReallyDoSomething. – Rentfree

Answer 3

You can check the value of done before and after the lock:

    if (!done)
    {
        lock(this)
        {
            if(!done)
            {
                done = true;
                _DoSomething();
            }
        }
    }

This way you won't enter the lock if done is true. The second check inside the lock is to cope with race conditions if two threads enter the first if at the same time.

BTW, you shouldn't lock on this, because it can cause deadlocks. Lock on a private field instead (like private readonly object _syncLock = new object())

Necropsy

Note also that double-checked locking is broken in some hardware/software architectures. In CLR, it is implemented correctly and allows for such usage. – Sita

Looks good. One caveat...done needs to be marked as volatile. – Bradytelic

lock creates a memory barrier, so skipping the volatile won't cause weird errors, only a possible performance hit: Without volatile the code might enter the outer if (!done) section and contest the lock when it doesn't really need to. Once the lock has been acquired then the code will see the up-to-date value of done in the inner if (!done) check. – Monoplane

@LukeH, that's what I thought, but I wasn't sure... thanks for the clarification. – Necropsy

@Groo, @LukeH, @Thomas: I do not believe any of your analyses are correct. @Brian: done does not need to be marked as volatile, though that solves the problem. What needs to happen is the first read of done must be performed with volatile semantics. It is not necessary for the second read to be volatile or for the write to be volatile. – Mendel

@Eric: But if the first read is not volatile, the only thing that can happen is that several threads could end up wait at lock, and then return as soon as they are granted the lock. I was referring to the general problem of double-checked locking due to compiler reordering of statements, which apparently isn't a problem in .NET (as described at the bottom of this article). – Sita

@Groo: Why is that the "only" thing that could happen given that we have not been shown either the implementation of _DoSomething or the implementation of the caller? What prevents the caller from accessing memory mutated by DoSomething? And what prevents those non-volatile memory accesses from being moved around in time to before the first read of "done" on a thread that never enters the lock (because "done" is true)? I don't think any of those things are prevented. However, I am not an expert on this subject; if you'd like to show me where the error is in my analysis I'd appreciate it. – Mendel

@Groo: re, that MSDN article. That is a completely different pattern. I know it looks the same, but from the processor's perspective that is completely different code. The pattern on the MSDN page only reads from one storage location and checks it for null. The code here could read from two storage locations which might be on completely different memory pages. Being four bytes apart is as good as being a billion bytes apart if those four bytes straddle a page boundary. The MSDN version is safe; clearly it's impossible to get inconsistent read order of a single location. – Mendel

@Eric, @Thomas: You are right Eric, I was talking nonsense. I didn't take enough time to think about the code posted. I had the singleton construction pattern in mind, with a null check of the field, and the newly created instance assigned inside the lock. Actually, the way this code is written right now is wrong, since done is set to true before _DoSomething is even executed, so there may be a number of threads which would presume that work was done. – Sita

@Groo: Right, that's the larger problem; I posted a comment a few minutes ago to the OP asking why they want to return early if the work is still being done. That seems like a misfeature to me. – Mendel

I believe the proper way is not only to put the done = true statement at the end, but to add a System.Threading.Thread.MemoryBarrier(); statement after the call to _DoSomething. Otherwise, if the method gets inlined and statements reordered, done might be set to true too early. – Sita

Answer 4

The lock keyword is just syntactic sugar for the Monitor class. Also you could call Monitor.Enter(), Monitor.Exit().

But the Monitor class itself has also the functions TryEnter() and Wait() which could help in your situation.

Gasoline

Answer 5

I know this answer comes several years late, but none of the current answers seem to address your actual scenario, which only became apparent after your comment:

The other threads don't need to use any information generated by ReallyDoSomething.

If the other threads don't need to wait for the operation to complete, the second code snippet in your question would work fine. You can optimize it further by eliminating your lock entirely and using an atomic operation instead:

private int done = 0;
public void DoSomething()
{
    if (Interlocked.Exchange(ref done, 1) == 0)   // only evaluates to true ONCE
        _DoSomething();
}

Furthermore, if your _DoSomething() is a fire-and-forget operation, then you might not even need the first thread to wait for it, allowing it to run asynchronously in a task on the thread pool:

int done = 0;

public void DoSomething()
{
    if (Interlocked.Exchange(ref done, 1) == 0)
        Task.Factory.StartNew(_DoSomething);
}

Sportswear

Answer 6

Because I came into a similar problem, but several function calls can be needed, which should be decided on inside the lock block, but should be executed after unlocking, I implemented a rather general approach:

    delegate void DoAfterLock(); // declare this outside the function
    public void MyFunction()
    {
        DoAfterLock doAfterLock = null;
        lock (_myLockObj)
        {
            if (someCondition)
                doAfterLock = () => DoSomething1(localParameters....);
            if (someCondition2)
                doAfterLock = () => DoSomething2(localParameters....);
        }

        doAfterLock?.Invoke();
    }

Anesthetist