41

I am a bit new in threading in c# and on general, in my program I am using mutex to allow only 1 thread getting inside a critical section and for unknown reason with doing some cw prints I can see that more than 1 thread is getting inside my critical section and this is my code : 

Mutex m = new Mutex(); m.WaitOne(); <C.S> // critical section here m.ReleaseMutex();

I would very much like to know if I am doing a mistake here thanks in advance for your kind help.

EDIT:

My code include classes so it basically looks more like this:

public class test { private mutex m; public test() { m = new mutex(); } public func() { m.WaitOne(); <C.S> // critical section here m.ReleaseMutex(); } }
Improve this question
4
  • You must have more than one instance of the class. Commented Apr 22, 2011 at 11:37
  • 3
    That is an instance-level mutex; are you sure your coding isn't in the critical section on separate unrelated instances? Also, any error will render the code permanently locked - you need a try/finally for that to be safe. Commented Apr 22, 2011 at 11:38
  • 1
    After the Edit: this Mutex works only at the object level, ie 1 CrticialSection / instance. Is that what you want? Commented Apr 22, 2011 at 11:52
  • Info: Mutex is mutual exclusion Commented Apr 11, 2020 at 12:52

5 Answers 5

Reset to default
73

The problem here is that all your callers are using a different mutex; you need the locking object to be shared, usually by making it a field. For example, and switching to a simpler lock metaphor:

private readonly object syncLock = new object(); public void ThreadSafeMethod() { lock(syncLock) { /* critical code */ } }

or using the mutex:

private readonly Mutex m = new Mutex(); public void ThreadSafeMethod() { m.WaitOne(); try { /* critical code */ } finally { m.ReleaseMutex(); } }
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

the mutex that i am using is a global mutex of a class so i think that it is the same mutex
@Nadav - that is not what your question shows... your question shows it as the same location as code, i.e. a local variable.
Nadav - are you sure you don't have more than one instance of the class? Almost all 'lock/mutex is letting multiple calls through' problems turn out to be that there are more lock/mutex objects than you intended.
5

It looks like you give each Thread its own Mutex. That won't work.

And a Mutex is overkill in most situations. You only need:

private static object syncLock = new object(); // just 1 instance .... lock(syncLock) { // critical section }
Improve this answer

2 Comments

I'm not sure from the question that we can infer static is intended... although we can't infer that it isn't either ;p
@Marc, correct. I used static as the opposite of what I think is the error. The code is not entirely clear.
5

This pattern does no locking at all. Every thread creates a new Mutex object and immediately owns the lock for it. Other threads create and use a new Mutex itself.

Consider using a regular lock()! 

lock(_lockobject) { // do inside what needs to be done - executed on a single thread only }

where _lockobject is a simple private variable in your class: 

private object _lockobject;

Edit: thanks to the commenters! Situations exist, where lock(this) can be dangerous. So I removed that.

Improve this answer

4 Comments

Do not lock(this), or lock(someType), or lock(anyString) - those are bad designs and are not robust.
BTW, locking this is not allways good idea, better is create simple variable of type object toolazy.me.uk/…
lock(this) is not considered to be good practice. (See, for example haacked.com/archive/2006/08/08/ThreadingNeverLockThisRedux.aspx). Much better to make an Object just for locking on, and forget that the .NET design error that lets any object be locked on was ever made.
@WillDean - indeed; I'd rather that there was a specific type, for example Monitor instances. Also, object should be abstract ;p
2

Mutex use to identify run app instance. 

 using (Mutex mutex = new Mutex(true, "app name", out createdNew)) { if (createdNew)//check app is already run { KillOthers(); StartApp(); } else { MessageBox.Show("Another instance already running!"); } }
Improve this answer

Comments

-2

May i add a correction to the accepted answer?

private readonly Mutex m = new Mutex(); public void ThreadSafeMethod() { while(!m.WaitOne()){} try { /* critical code */ } finally { m.ReleaseMutex(); } }
Improve this answer

1 Comment

Hi Emanuele. Why do you think that your answer is an improvement? The loop: while (!m.WaitOne()) { } is never going to run more than once, because the WaitOne method with no arguments always returns true (unless it never returns).

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.