current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I am trying to make a thread lock for a method so that only one thread can run it at a time. Although this is a well known issue and in C# there is the lock keyword to do it. I was trying to do it on my own and try to understand the depth of the problem of syncing multiple threads around one method.

private bool free = true;

public void start()
{

if(free== true)
{
 free= false;

 await DoTheWork();

 free = true;
}


}

But I know that there might be a racing condition on the first line which is if(free==true) and two threads can enter that statement block. So I added a double lock which is the counter as follow:

private bool free = true;
private int counter =0;
public void start()
{

if(free== true)
{
 free= false;
 counter++;

 if(counter>1) return;
 await DoTheWork();

 free = true;
}


}

I know that designing an algorithm from scratch while there is a ready made library is not a good practice and it is considered (reinventing the wheel)... but for learning purposes I am interested to know what are the flaws in the algorithm mentioned above. Am I ok with that?

share|improve this question

closed as off-topic by svick, 200_success♦ Apr 27 at 21:32

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.
    
Your code won't compile, you can't use await in a method that is not async. –  svick Apr 27 at 19:31
    
Svick , thanx ,, already corrected –  stackunderflow Apr 28 at 5:29
1  
I am rolling back the fix you applied to your code. This is because it invalidates the current answer. Please see: What you may and may not do after receiving answers Note that, in this particular question, I can't see a way to make this question on-topic again without invalidating the answer. It was a question that perhaps should have been closed immediately, and fixed then. –  rolfl Apr 28 at 11:19

1 Answer 1

up vote 4 down vote accepted

Unfortunately your code doesn't work.

counter++;

Is not atomic which means that it can be interrupted when it's halfway done. The Interlocked.Increment(ref int) method can be used to protect against this but that won't help you here.

You can also get into the situation where counter is greater than 1 for all your threads thus locking you out forever.

Thread A calls start and evaluates free to be true. Immediately afterwards, another thread B calls start and also finds the free variable true so it happily sets free to false and then increments the counter. Thread A then also sets the free variable to false and increments the counter, it finally checks the counter and finds it to be greater than 1 so it exits without doing the work. Thread B starts again where it left off and checks the counter variable - it's greater than 1 so it too exits without doing the work.

Your free and counter variables are now stuck in a state where the work will never be done. Bad times :(

Implementing lock free code is really, really hard and you'll end up leaning heavily on the Interlocked class. This would be so simple with a lock:

private static readonly object syncRoot = new Object();

public void Start()
{
    if (!Monitor.TryEnter(syncRoot))
    {
        // Can't get the lock, something else must already be doing the work.
        return;
    }
    try
    {
        DoTheWork();
    }
    finally
    {
        // ALWAYS make sure we release the lock.
        Monitor.Exit(syncRoot);
    }
}

Notes about your code:

  • You can't await without the method being marked as async.
  • Method names should be PascalCase: start -> Start
  • if (free == true) is better written as if (free)
share|improve this answer
    
I like your answer, however it doesn't deal with the Async methodology.. as described by Jon Skeet and Stephen Cleary here stackoverflow.com/questions/15633685/… please update your answer for using the SemaphoreSlim.WaitAsync so I can mark it as the accepted answer –  stackunderflow Feb 24 at 14:41
1  
@stackunderflow - Your code had an await without marking the method async. I had to decide whether or not you meant to have the await or not. Given the stub nature of your code, I went for the simpler option of no async as it was easier to explain why your code wouldn't work. Stephen Cleary has an excellent article here: msdn.microsoft.com/en-us/magazine/jj991977.aspx which includes the correct use of SemaphoreSlim –  RobH Feb 24 at 14:55
    
@stackunderflow - as a bit of a hint: SemaphoreSlim.Wait(0) behaves very similarly to Monitor.TryEnter(obj) –  RobH Feb 24 at 15:03
    
Thank you for the invaluable response –  stackunderflow Feb 24 at 16:06

Not the answer you're looking for? Browse other questions tagged or ask your own question.