What is the correct way to block threads?

Question

In my MVC 3 C # application, I have some static object that I want to be available one request at a time. It can only be accessed using methods, but I want the lock to be maintained between calling its methods.

Calls will be made only in the controller, usually there will be one or two blocked blocks of code.

At first I wanted to expose some static public object and use it just as

lock(MyClass.lockObject) { MyClass.doStuff(); MyClass.doStuff2(); } 

but I find him prone to errors, as I can forget to block him somewhere. I wonder if this is the correct way to use Monitor.Enter() in the constructor and Monitor.Exit() in the Dispose method, and then change my methods to non-static? Say something like:

 public class MyClass:IDisposable { static protected object _locker = new object(); protected bool isDisposed = false; public MyClass() { Monitor.Enter(_locker); } public void Dispose() { if (!isDisposed) { Monitor.Exit(_locker); GC.SuppressFinalize(this); isDisposed = true; } } ~SPInstances() { Dispose(); } public void doStuff() { if(isDisposed) throw new ObjectDisposedException(); // do stuff here, etc. } } 

Then I could use it like:

 using(MyClass myinstance = new MyClass()) { myInstance.doStuff(); myInstance.doStuff2(); } 

Then, even if I forgot to wrap the code during use, it will still be blocked and there will be some chance that it will be unlocked during garbage collection ...

I don’t own C # and sometimes I don’t see some aspects, and threads will never be easy to debug later, so I wonder if I am well on a good track. Is this the right way to achieve my goal?

EDIT:

Extending the idea of ​​Master Morality, will this method be better (I simplified it a bit, since I only need one instance of the resource)?

 public class MyClass { static protected readonly MyResourceType _myResourceStatic = new MyResourceType(); static public void DoWork(Action<MyClass> action) { lock(_myResource) { action(new MyClass(_myResource)); } } protected MyClass(MyResourceType myResource) { _myResource = myResource; } protected readonly _myResource; public void DoFirstThing() { ... } public void DoSecondThing(){ ... } } MyClass.DoWork(x => { x.DoFirstThing(); // do unrelated stuff x.DoSecondThing(); }); 

Answer 1

IMHO it is better to lock inside your own methods. Thus, another programmer or himself later does not need to remember lock before they call the method, and this is quite simple.

 public class MyClass { private static readonly object _gate = new object(); /* something that can only be accessed by one thread at a time...*/ private static MyResourceType MyResource = new MyResourceType(); public void DoSomething() { lock(_gate) { /* do something with MyResource, just make sure you DO NOT call another method that locks the gate ie this.DoSomethingElse(), in those situations, you can take the logic from DoSomethingElse() and toss it in a private method ie _DoSomethingElse(). */ } } private void _DoSomethingElse() { /* do something else */ } public void DoSomethingElse() { lock(_gate) { _DoSomethingElse(); } } } 

On the same day...

 var myClass = new MyClass(); myClass.DoSomething(); 

if you want to be able to call multiple methods with a lock, you can do it with a lambda, and to be really safe, wrap it in a helper class.

 public class MyClass { public MyResourceType MyResource { get; set; } public void DoFirstThing() { ... } public void DoSecondThing(){ ... } } public class MyClassHelper { private static readonly object _gate = new Object(); private static MyResourceType MyResource = new MyResourceType(); private MyClass _myClass = new MyClass(); public void DoWork(Action<MyClass> action) { lock(_gate) { _myClass.MyResource = MyResource; action(_myClass); _myClass.MyResource = null; } } } ... var myClassHelper = new MyClassHelper(); myClassHelper.DoWork(x => { x.DoFirstThing(); x.DoSecondThing(); }); 

Answer 2

locking is simpler and less prone to errors when using Monitor.Enter and Exit directly.

It’s not clear from your example that you are trying to sync.

It is not recommended to use Monitor.Enter in the constructor and Exit in Dispose. You will have to handle all exceptions inside c'tor and call Exit if you cannot build the class correctly. It does not make sense to lock the instance, which essentially means c'tor lock. You can see the Synchronized attribute; but I don’t think it is really recommended.

Answer 3

How important is it that requests from other objects to your static object are executed immediately? You can achieve mutual exclusion by isolating threads if you have a static object that supports the queue through which it runs. When called from another object, the requested work is placed in the queue, while in a separate thread a static object works through the queue (note the need for mutually exclusive access to the queue, though!), Performing requests.

You can either have a block of callers in the method that added the job to the queue until it is notified by the static object, or provide a callback interface so that the static object can notify callers that their work has been completed.

Answer 4

From your example it is not clear what you are trying to do. As a good programming practice, it is best for each individual method to receive a lock and release it when it is done with a critical section. In your case, it will be:

 void doStuff() { if(isDisposed) throw new ObjectDisposedException(); // do stuff here, etc. lock(_locker) { // enter critical section here } // continue to do other stuff } void doStuff2() { if(isDisposed) throw new ObjectDisposedException(); // do stuff here, etc. lock(_locker) { // enter critical section here } // continue to do other stuff } 

Now locking is a short version for using the Monitor class. And actually translated as:

 bool getLock = false; try { Monitor.Enter(locker, ref getLock); // do stuff here } finally { if(getLock) { Monitor.Exit(locker); } } 

This will give you more control over the state of your object and internal representation, as if something is wrong there, you can return to the previous state of your object.

Answer 5

If there are not many combinations of calls, you can make dostuff and doStuff2 private and put a wrapper function with a lock in your class

 static public void doStuffs() { lock (lockObject) { doStuff(); doStuff2(); } }