views:

51

answers:

3

I'm trying to find a way to provide exclusive access to a resource, while at the same time providing information about the state of the lock (_isWorking) for other classes to read.

Here's what I have come up with so far :

    private int _isWorking = 0;

    public bool IsWorking
    {
        get { return Interlocked.CompareExchange(ref _isWorking, 0, 0) == 1; }
    }

    public void LaunchProcess()
    {
        if (Interlocked.CompareExchange(ref _isWorking, 1, 0) == 0)
        {
            try
            {
                DoExpansiveProcess();
            }
            finally
            {
                Interlocked.Exchange(ref _isWorking, 0);
            }
        }
    }

    public void DoExpansiveProcess()
    {
        Thread.Sleep(30000);
    }

It does not work exactly as a lock since the second thread, upon seeing that a thread is already running the process, will just leave the method and do nothing.

Is it the correct way to do this or are there any C# 3.5 structure more adapted for the purpose?

And how to implement a 'real' lock scenario, where the second thread would still execute the method after the first one finishes, while providing information about the state?

A: 

System.Threading.ManualResetEvent might be able to help here. Just do a WaitOne() against it. If it's being used then it will come back as false. If it isn't in use it is available.

One of the WaitOne() overloads takes a time in milliseconds to wait. If you use WaitOne(0) then the wait will return immediately in a non blocking way.

You should do some research into the synchronisation primitives that .Net offers:

System.Threading.Monitor lock() { }
System.Threading.ManualResetEvent
System.Threading.AutoResetEvent
System.Threading.Semaphore
System.Threading.Mutex
Spence
But I would need something like in between ManualResetEvent and AutoResetEvent:ManualResetEvent because I don't want that accesses to IsWorking resets the waithandle.AutoResetEvent to implement the locking part, to have the Reset inside the WaitOne().
lau
The synchronization primitives avoid providing that information. Reason is that almost any code that relies on a primitive being signaled suffers from race conditions (after the check the state changes). The safest way is for the code to share the lock to confirm that a thread is in a given state.
Spence
A: 

I think your approach should in general be correct and will definitely be much faster than using a ManualResetEvent (if its at all relevant) - because ManualResetEvent wraps underlying unmanaged primitive.

To make this approach safe however, _isWorking has to be private.

Grzenio
I changed _isWorking to private, thanks.The CompareExchange trick in IsWorking property is to ensure that we read the latest value of _isWorking. I could have declared the field as volatile, but I wanted to avoid this annoying VS warning.see : http://stackoverflow.com/questions/425132/a-reference-to-a-volatile-field-will-not-be-treated-as-volatile-implications/425145#425145
lau
Nice trick, didn't know that! I will edit my answer accordingly
Grzenio
A: 

Assuming that I understand your question correctly then I think what you are after is the Monitor.TryEnter method with a timeout value of 0. It will attempt to acquire the lock, but with a timeout of 0 it will always return immediately with value indicating whether the lock was actually acquired. The implication here is that if the lock was not acquired then it is assumed that it is already acquired by someone else. The problem is that if returns true then you would have to immediately release it with a call to Monitor.Exit.

Perhaps this would be a better solution:

public class Worker
{
  private Object m_LockObject = new Object();
  private volatile bool m_IsWorking = false;

  public bool IsWorking
  {
    get { return m_IsWorking; }
  }

  public void LaunchProcess()
  {
    lock (m_LockObject)
    {
      m_IsWorking = true;
      DoExpansiveProcess();
      m_IsWorking = false;
    }
  }
}
Brian Gideon
What annoys me here is that taking the lock and setting m_IsWorking to true is not an atomic operation.
lau
Ah, I think I understand what you are asking now. I edited to my answer to include a solution using `Semaphore` which I believe replicates the atomimicity you are after. Of course from outside of the `Worker` class a caller who executes `IsWorking` and then `LaunchProcess` would still race with another thread doing the same thing. That caller would have to implement an independent synchronization scheme. So in that respect it is no different than the solution using the volatile flag right?
Brian Gideon
Hmm...now that I think about it the `Semaphore.WaitOne` method would have the same problem as `Monitor.TryEnter`. That is it might actually take a count from the semaphore forcing you to have to release it immediately if the call returned true.
Brian Gideon
Well, IsWorking flag is not meant to be used to check if it is working and then launch the process. It would be used only to inform about the state of the process. It would be dangerous to use it for synchronization purpose.But I guess your "TryEnter(0) and then Exit" to check the state of the lock would do the trick.
lau