tags:

views:

1568

answers:

4

Wouldn't this be overkill and only one of these necessary? I've searched and found different posts about Mutual Exclusion and locks in C# here and here.

Example:
In our app, we have a function that spins off multiple reconnection threads and inside this thread we use a Mutex and a lock. Wouldn't lock block access to this section of code and prevent connect from being updated by any other thread?

bool connect = false;
Mutex reconnectMutex = new Mutex(false, "Reconnect_" + key);

try
{
   lock(site)
   {
      if(site.ContainsKey(key))
      {
         siteInfo = (SiteInfo)site[key];
         if(reconnectMutex.WaitOne(100, true))
         {
            connect = true;
         }
      }
   }

   if (connect)
   { 
      // Process thread logic
   }
}
catch
{}

reconnectMutex.ReleaseMutex();

More Info:
This is in an ASP.NET WebService not running in a Web Garden.

+8  A: 

That Mutex (because it has a name) will stop any process on the same machine accessing it as well, whereas lock will only stop other threads in the same process. I can't see from that code sample why you'd need both kinds of lock. It seems good practice to hold the simple lock for a short period of time - but then the much heavier interprocess mutex is locked for a probably longer (though overlapping) period! Would be simpler to just use the mutex. And perhaps to find out whether an interprocess lock is really necessary.

By the way, catch {} is absolutely the wrong thing to use in that scenario. You should use finally { /* release mutex */ }. They are very different. The catch will swallow far more kinds of exception than it should, and will also cause nested finally handlers to execute in response to low-level exceptions such as memory corruption, access violation, etc. So instead of:

try
{
    // something
}
catch
{}

// cleanup

You should have:

try
{
    // something
}
finally
{
    // cleanup
}

And if there are specific exceptions you can recover from, you could catch them:

try
{
    // something
}
catch (DatabaseConfigurationError x)
{
    // tell the user to configure the database properly
}
finally
{
    // cleanup
}
Daniel Earwicker
it has to be named accordingly to be machine-specific, though.
Marc Gravell
Good point, I've hopefully clarified that in the first sentence.
Daniel Earwicker
Are you saying I should eliminate the catch statement and replace it with a finally?
Jeremy Cron
Yes. You may need a catch as well if the "Process thread logic" code throws something you can recover from - but then you should only catch specifically what you can recover from. And generally speaking, "catch {} Foo();" is the wrong way to achieve what "finally { Foo(); }" is designed to do.
Daniel Earwicker
I've put this more clearly in the answer, even though it's off topic - it's probably more important than the duplicate locking problem!
Daniel Earwicker
+2  A: 

"lock" is basically just a syntactic sugar for Montor.Enter/Exit. Mutex is a multi-process lock.

They have very different behavior. There is nothing wrong with using both in the same application or methods, since they're designed to block different things.

However, in your case, I think you may be better off looking into Semaphore and Monitor. It doesn't sound like you need to lock across processes, so they are probably a better choice in this situation.

Reed Copsey
+1  A: 

You didn't give enough info to really answer this. As already stated by Earwicker a Mutex allows you to have a synchronization accross processes. Thus if you have two instances of the same app running you can serialize access. You might do this for example when using external resources.

Now you lock on site protects site from access by other threads in the same process. This might be nessecary depending on what other methods / threads are doing. Now if this is the only place that site is being locked then yes I would think it is overkill.

JoshBerke
+1  A: 

As others have pointed out, the Mutex locks across processes and the local lock (Monitor) locks only those threads owned by the current process. However ...

The code you showed has a pretty serious bug. It looks like you're releasing the Mutex unconditionally at the end (i.e. reconnectMutex.ReleaseMutex()), but the Mutex is only acquired if site.ContainsKey() returns true.

So if site.ContainsKey returns false, then releasing the Mutex is going to throw ApplicationException because the calling thread does not own the Mutex.

Jim Mischel
Thanks - I found that bug after I posted this. I couldn't figure out why the app was just closing - that was the problem.
Jeremy Cron