How to make a method exclusive in a multithreaded context?

Question

I have a method which should be executed in an exclusive fashion. Basically, it's a multi threaded application where the method is invoked periodically by a timer, but which could also be manually triggered by a user action.

Let's take an example :

1. The timer elapses, so the method is called. The task could take a few seconds.

2. Right after, the user clicks on some button, which should trigger the same task : BAM. It does nothing since the method is already running.

I used the following solution :

public void DoRecurentJob()
{
    if(!Monitor.TryEnter(this.lockObject))
    {
        return;
    }

    try
    {
        // Do work
    }
    finally 
    {
        Monitor.Exit(this.lockObject);
    }
}

Where lockObject is declared like that:

private readonly object lockObject = new object();

Edit : There will be only one instance of the object which holds this method, so I updated the lock object to be non-static.

Is there a better way to do that ? Or maybe this one is just wrong for any reason ?

Answer 1

We have a similar requirement, with the added requirement that if the long-running process is requested again, it should enqueue to perform another cycle after the current cycle is complete. It's similar to this:

https://codereview.stackexchange.com/questions/16150/singleton-task-running-using-tasks-await-peer-review-challenge

private queued = false;
private running = false;
private object thislock = new object();

void Enqueue() {
    queued = true;
    while (Dequeue()) {
        try {
            // do work
        } finally {
            running = false;
        }
    }
}

bool Dequeue() {
    lock (thislock) {
        if (running || !queued) {
            return false;
        }
        else
        {
            queued = false;
            running = true;
            return true;
        }
    }
}

Answer 2

A more declarative way of doing this is using the MethodImplOptions.Synchronized specifier on the method to which you wish to synchronize access:

[MethodImpl(MethodImplOptions.Synchronized)] 
public void OneAtATime() { }

However, this method is discouraged for several reasons, most of which can be found here and here. I'm posting this so you won't feel tempted to use it. In Java, synchronized is a keyword, so it may come up when reviewing threading patterns.

Answer 3

I think Microsoft recommends using the lock statement, instead of using the Monitor class directly. It gives a cleaner layout and ensures the lock is released in all circumstances.

public class MyClass
{

  // Used as a lock context
  private readonly object myLock = new object();

  public void DoSomeWork()
  {
    lock (myLock)
    {
      // Critical code section
    }
  }
}

If your application requires the lock to span all instances of MyClass you can define the lock context as a static field:

private static readonly object myLock = new object();

Answer 4

The code is fine, but would agree with changing the method to be static as it conveys intention better. It feels odd that all instances of a class have a method between them that runs synchronously, yet that method isn't static.

Remember you can always have the static syncronous method to be protected or private, leaving it visible only to the instances of the class.

public class MyClass
{ 
    public void AccessResource()
    {
        OneAtATime(this);
    }

    private static void OneAtATime(MyClass instance) 
    { 
       if( !Monitor.TryEnter(lockObject) )
       // ...

Answer 5

This is a good solution although I'm not really happy with the static lock. Right now you're not waiting for the lock so you won't get into trouble with deadlocks. But making locks too visible can easily get you in to trouble the next time you have to edit this code. Also this isn't a very scalable solution.

I usually try to make all the resources I try to protect from being accessed by multiple threads private instance variables of a class and then have a lock as a private instance variable too. That way you can instantiate multiple objects if you need to scale.

Answer 6

Minor nit: if the lockObject variable is static, then "this.lockObject" shouldn't compile. It also feels slightly odd (and should at least be heavily documented) that although this is an instance method, it has distinctly type-wide behaviour as well. Possibly make it a static method which takes an instance as the parameter?

Does it actually use the instance data? If not, make it static. If it does, you should at least return a boolean to say whether or not you did the work with the instance - I find it hard to imagine a situation where I want some work done with a particular piece of data, but I don't care if that work isn't performed because some similar work was being performed with a different piece of data.

I think it should work, but it does feel a little odd. I'm not generally a fan of using manual locking, just because it's so easy to get wrong - but this does look okay. (You need to consider asynchronous exceptions between the "if" and the "try" but I suspect they won't be a problem - I can't remember the exact guarantees made by the CLR.)

Answer 7

You could also use Mutex or Semaphore if you want it to work cross process (with a slight performance penalty), or if you need to set any other number than one of allowed simultaneous threads running your piece of code.

There are other signalling constructs that would work, but your example looks like it does the trick, and in a simple and straightforward manner.

Answer 8

This looks reasonable if you are just interested in not having the method run in parallel. There's nothing to stop it from running immediately after each other, say that you pushed the button half a microsecond after the timer executed the Monitor.Exit().

And having the lock object as readonly static also make sense.