views:

282

answers:

8

I have several sections of code that I need to protect with a Mutex. The problem is that the code looks something like this:

lock(mylockobject) {
  if(!foo())
    throw new MyException("foo failed");
  if(!bar())
    throw new MyException("bar failed");
}

Using lock, it works as I'd like, but now I need to use a mutex. The obvious problem here is that if I acquire the mutex and foo() or bar() fails, I would have to explicity release the mutex before throwing each exception.

In C++, I would take advantage of the scope of an object created on the stack, and would lock the mutex in the object's constructor, and then release it in the destructor. With .NET's garbage collection, I didn't think this would work. I wrote a test app and confirmed that if I do something like this:

public class AutoMutex
{
  private Mutex _mutex;
  public AutoMutex(Mutex mutex)
  {
     _mutex = mutex;
     _mutex.WaitOne();
  }

  ~AutoMutex()
  {
    _mutex.ReleaseMutex();
  }
}

and then have code like this:

// some code here...
Mutex my_mutex = new Mutex(false, "MyMutex");
{ // scoping like I would do in C++
  AutoMutex test = new AutoMutex(my_mutex);
  test = null;
}

The destructor (finalizer?) doesn't get called until much later.

Google hasn't yet pointed me in the right direction, but I'm still working on it... Please let me know how you might solve this little problem, if it's even possible.

+9  A: 

Mutex implements IDisposable so wrap it in a using

using (Mutex m = new Mutex())
{
   //Use mutex here

}
//Mutex is out of scope and disposed
n8wrl
@Oded: If by "is isn't" you mean "`Mutex` doesn't implement `IDispoable" then you are mistaken. `Mutex` inherits from `WaitHandle` which implements `IDisposable`.
Jason
@Oded-- it is because it extends WaitHandle and WaitHandle implements IDisposable. Doesn't do much good here, though, unless AutoMutex extends Mutex instead of using it via composition.
tvanfosson
It is, Mutex extends Waithandle, which implements IDisposable. Yay inheritance!
C. Ross
@Oded: Yes it does.
Steven
@tvanfosson, I think he was suggesting creating his own mutex class because he didn't understand how to use the delivered one.
C. Ross
@C.Ross -- that's certainly possible, though, he might also be extending it to give it explicit semantics. Hard to tell.
tvanfosson
C. Ross is correct, I didn't realize the connection between instant garbage collection, IDisposable, and the using keyword. That's why I was trying to do it like I always did in C++. I really like this particular approach and will use it first.
Dave
Oh, I re-read C. Ross's comment and actually I wasn't trying to create my own mutex class. I was trying to create something that could control the lifespan of the Mutex object. So right now, I can't test very well because I can't derive a class from Mutex (I want to do that to track the finalizer), but I'll figure something out.
Dave
+7  A: 

Use a try/finally block or use the IDisposable pattern and wrap your usage in a using statement.

tvanfosson
I'll try this! Seems like a lot of people have the same suggestion. I'm glad that this has also opened my eyes a lot and now have an idea of how to deal with such a situation in the future (i.e. follow inheritance path to things like IDisposable)
Dave
+1  A: 

Why don't you surround the code in Foo() in a try-catch-finally, then have the finally release the mutex? (The catch can rethrow any exceptions, wrapping them in your custom exception type if desired.)

Pat
+8  A: 

In order to provide scoping, you can make your AutoMutex implement IDisposable and use it like this:

using(new AutoMutex(.....))
{
  if(!foo())
    throw new MyException("foo failed");
  if(!bar())
    throw new MyException("bar failed");
}    

In your implementation of IDisposable.Dispose(), release the mutex.

Marcel Gosselin
Although everyone offered very similar advice, Marcel, yours is the answer that worked perfectly for me. Now my AutoMutex works exactly the way I'd expect it to!!! Thanks!
Dave
A: 

For scope-based action, the C# idiom is try...finally. It would look like this:

try {
    acquire the mutex
    do your stuff which may fail with an exception
} finally {
    release the mutex
}

Using object instances for resource acquisition is a C++-ism which comes from the ability of C++ to handle instances with a life limited by the current scope. In languages where instances are allocated in a heap and destroyed asynchronously by a garbage collector (C#, Java), destructors (aka finalizers) are not the right tool for that. Instead, the finally construct is used to perform scope-based action.

Thomas Pornin
+2  A: 

I think everyone has you covered with dispose/using, but here's an example using try/finally:


Mutex m = new Mutex()
m.WaitOne();
try
{
  if(..)
    throw new Exception();              
}
finally
{
  // code in the finally will run regardless of whether and exception is thrown.
  m.ReleaseMutex();
}


JMarsch
+13  A: 

Couple points.

1) The thing you want to search for is "the disposable pattern". Be very careful to implement it correctly. Of course, Mutex already implements the disposable pattern, so its not clear to me why you'd want to make your own, but still, it's good to learn about.

See this question for some additional thoughts on whether it is wise to use the disposable pattern as though it were RAII:

http://stackoverflow.com/questions/2101524

2) Try-finally also has the semantics you want. Of course a "using" block is just a syntactic sugar for try-finally.

3) Are you sure you want to release the mutex when something throws? Are you sure you want to throw inside a protected region?

This is a bad code smell for the following reason.

Why do you have a mutex in the first place? Usually because the pattern goes like this:

  • state is consistent but stale
  • lock access to the state
  • make the state inconsistent
  • make the state consistent
  • unlock access to the state
  • state is now consistent and fresh

Consider what happens when you throw an exception before "make the state consistent". You unlock access to the state, which is now inconsistent and stale.

It might be a better idea to keep the lock. Yes, that means risking deadlocks, but at least your program isn't operating on garbage, stale, inconsistent state.

It is a horrid, horrid thing to throw an exception from inside a lock-protected region and you should avoid doing so whenever possible. An exception thrown from inside a lock makes you have to choose between two awful things: either you get deadlocks, or you get crazy crashes and unreproducible behaviour when your program manipulates inconsistent state.

The pattern you really ought to be implementing is:

  • state is consistent but stale
  • lock access to the state
  • make the state inconsistent
  • make the state consistent
  • if an exception occurs, roll back to the stale, consistent state
  • unlock access to the state
  • state is now consistent and, if there was no exception, fresh

That's the much safer alternative, but writing code that does transactions like that is tough. No one said multithreading was easy.

Eric Lippert
It's a little strange to explain, but the basic reason why I have to use a mutex is that I'm using a 3rd party C DLL that is not threadsafe. It's a DLL that allows me to send commands over a communication network, and each in-process server that I write needs to have its own copy of the DLL. However, they must be using statics somewhere, because locking doesn't help. The only other temporary solution I can come up with is to do a system-wide lock like a mutex. In the end, I think their suggestion to have separate copies of the DLL is not necessary. As two processes, everything works fine.
Dave
@Eric: +1 for a deeper answer to the question. For the pattern you describe at the end, I generally look to use something like a copy-on-write approach, to simplify handling state rollback during an exception. It also allows most of the operation to be performed *outside* of a mutex, which improves performance. http://en.wikipedia.org/wiki/Copy-on-write
LBushkin
+2  A: 

The GC isn't deterministic. Also, it behaves differently in debug mode, making this a bit more confusing to deal with in a development environment.

To create and use your auto mutex in the way you wish, implement IDisposable and use the using keyword to destroy/release your AutoMutex when it goes out of scope.

public class AutoMutex : IDisposable
{
    private Mutex _mutex;  
    public AutoMutex(Mutex mutex)
    {
       _mutex = mutex;
       _mutex.WaitOne();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    void Dispose(bool disposing)
    {
        // method may be called more than once
        // use locking to make this thread safe
        if(_mutex != null)
        {
          _mutex.ReleaseMutex();
          _mutex.Dispose();
        }
        _mutex = null;
        if(disposing)
        {
          // release managed resources here
        }
    }
}

and in use

using(new AutoMutex(new Mutex(false, "MyMutex")))
{
  // whatever within the scope of the mutex here
}
Will