How to solve probable race condition with Mutex

Question

I have the class ComputationService which is the wrapper of the old legacy COM object. This COM object cannot have multiple instances because of how its implemented. In the application, there are two buttons (for simplification) when clicked a new Task runs which use a ComputationService these tasks are kind of long-running tasks so you can click on the second button when the first one is in the process of computing. That's where the problem occurs because I can't let both threads use the ComputationService so I decided to use Mutex to allow only one instance of ComputationService at the time.

What I have:

public class ComputationService : IComputationService
{
       public ComputationService()
       {
           _mutex = new Mutex(true, "AwesomeMutex");
           _mutex.WaitOne();
       }
       public void Dispose()
       {
           _mutex.ReleaseMutex();
       }
}

and then two places where I use it:

public Task Compute1()
{
   return Task.Run(() =>
   {
       foreach (var item in items)
       {
            using (var service = _computationFactory()) // new instance of ComputationService
            {
               //Do some work
            }
       }
   });
 }

public Task Compute2()
{
   return Task.Run(() =>
   {
       foreach (var item in items)
       {
            using (var service = _computationFactory()) // new instance of ComputationService
            {
               //Do some other work
            }
       }
   });
 }

What I get when I am debugging to console:

Mutex created by Thread: 18
Wait, thread: 18
Do Work, thread: 18
Mutex created by Thread: 20
Wait, thread: 20
Mutex release by thread: 18

Mutex created by Thread: 18
Wait, thread: 18
Do Work, thread: 18
Mutex release by thread: 18

Then of course AbandonedMutexException() occurs on Thread 20.

I tried to use lock in the constructor of ComputationService but did not work.

Can you please help me, what I am doing wrong and how to fix it?

Answer 1

Problem is here:

_mutex = new Mutex(true, "AwesomeMutex");
_mutex.WaitOne();

You are passing true as first argument, named initiallyOwned. If you pass true there - that means IF mutex with such name did not exist before and is created as a result of this constructor - current thread now owns it. You don't check if mutex was created as a result of this call, and actually there is even no way to check that when using this constructor overload. So never ever pass true if using this constructor.

If you want to utilize initiallyOwned - then use another overload, which actually tells you whether mutex was created as a result of this constructor:

_mutex = new Mutex(true, "AwesomeMutex", out var createdNew);

Now, if createdNew returned true - you do NOT need to call _mutex.WaitOne(), because as per above - current thread already owns the mutex. That's what happens in your case - sometimes you already own the mutex and then you call _mutex.WaitOne(). Now to release it you would have to call Release() twice, which you never do.

Alternatively, just pass false, then wait:

_mutex = new Mutex(false, "AwesomeMutex");
_mutex.WaitOne();

If the whole thing happens in the same process, then you don't need global mutex (which you now have because you pass a name) - you can do fine with a local one:

public class ComputationService : IDisposable {
    // one static instance
    private static readonly Mutex _mutex = new Mutex();
    public ComputationService()
    {
        _mutex.WaitOne();
    }
    public void Dispose() {
        _mutex.ReleaseMutex();
    }
}

There are other improvements that can be made, but they are out of scope of this question.

As mentioned in comments - in single process lock (Monitor.Enter \ Monitor.Exit) will be faster and achieve the same result in your case:

public class ComputationService : IDisposable {
    // one static instance
    private static readonly object _mutex = new object();
    public ComputationService()
    {
        Monitor.Enter(_mutex);
    }
    public void Dispose() {
        Monitor.Exit(_mutex);
    }
}