I wonder is there a better way to implement this "simple lock"

Question

Is there a better way to do implement a simple lock like below?

I only want to to the "DOSOMETHING" if it's not already being run. Should I be using reall locks here? if I use lock will that cause everything to queue up and wait for the lock to release? (that's not what I want!)

Thanks

  bool running = false;

  void DataDisplayView_Paint(object sender, PaintEventArgs e)
  {
    // if (!this.initialSetDone)  
     if (!running)
     {
        this.running = true;

        //DOSOMETHING

        this.running = false;
     }
 }

Answer 1

The best way is to use a try/finally block

try { 
  this.running = true;
  ...
} finally {
  this.running = false;
}

Real thread locks are only needed if this method is called from multiple threads. Given that it appears to be a paint event handler this is unlikely as controls are affinitized to a single thread.

Answer 2

No, you do not want to use locks here. This is not a thread synchronization problem. This is a method reentrancy problem.

You might try something like this.

bool running = false; 

void DataDisplayView_Paint(object sender, PaintEventArgs e) 
{ 
  if (!this.running)
  {
    this.running = true; 
    try
    {
      //DOSOMETHING 
    }
    finally
    {
      this.running = false; 
    }
  }
}

Answer 3

You just need to synchronise (lock is the simplest way) bits of the code:

bool running = false;
readonly object padlock = new object();

  void DataDisplayView_Paint(object sender, PaintEventArgs e)
  {

     if (!this.initialSetDone)
     {
        lock(padlock)
        {
          if(running) return;
          running = true;
        }
        try {

          //DOSOMETHING
        }
        finally
        {
          lock(padlock)
          {
            this.running = false;
          }
        }
     }
 }

Answer 4

If you use Monitor.TryEnter instead you could specify a timeout, in which case the result you get is such that:

• only one thread can run the DOSOMETHING at a time
• subsequent calls will try to get the lock and give up after the timeout clause

If you don't provide with a timeout, or set the timeout to 0, this call won't block and will return immediately (maybe that'd suit your requirement better?):

if (!this.initialSetDone && Monitor.TryEnter(_lock))
{
   // DOSOMETHING
}

Alternatively, you can make the running variable volatile so that you will always get the latest value stored in the variable:

private volatile bool running;

if (!this.initialSetDone && !this.running)  // #1
{
   this.running = true;
   try
   {
     // DOSOMETHING
   }
   finally
   {
     this.running = false;
   }
}

The second approach won't queue up subsequent calls, but there is the possibility that two threads will both hit #1 and evaluate that it's safe to proceed then both end up running DOSOMETHING, though it's highly unlikely.

Answer 5

You shift varaible names in the middle, so I'm going to assume you wanted:

  bool running = false; 

  void DataDisplayView_Paint(object sender, PaintEventArgs e) 
  { 
     if (!this.running) 
     { 
        this.running = true; 

        //DOSOMETHING 

        this.running = false; 
     } 
 }

The problem you have here is that if DataDisplayView_Paint can be called from multiple threads, then it is possible that between the if (!this.running) and the this.running = true; the other thread could jump in and start DOSOMETHING (because running is still false). Then the first thread will resume, and start DOSOMETHING again. If that is a possiblity, then you will need to use a real lock.

Answer 6

Am I missing something? The code as you've posted it does not seem to do anything. That is, the code will run whether or not running is true.

Generally, any code that tries to "lock" itself like this...

if (!running)
{
    running = true;

    try
    {
        // This code should not call itself recursively.
        // However, it may execute simultaneously on more than one thread
        // in very rare cases.
    }
    finally
    {
        running = false;
    }
}

...is perfectly good, as long as you're in a single-threaded scenario. If you're running multi-threaded code, problems can arise because you are assuming that no two threads will reach the if (!running) line at the same time.

The solution in multi-threaded code is to use some form of atomic switch. I've used the AutoResetEvent for this purpose:

var ready = new AutoResetEvent(true);

if (ready.WaitOne(0))
{
    try
    {
        // This code will never be running on more than one thread
        // at a time.
    }
    finally
    {
        ready.Set();
    }
}

Answer 7

Note that if you're having reentrancy on your paint callback, you've got a more serious problem. Paint handlers should be blocking your message pump (and should complete relatively quickly), so you should never see this case. The only exception is if you call Application.DoEvents() from somewhere in your paint handler, which you really shouldn't be doing.

Answer 8

I only want to to the "DOSOMETHING" if it's not already being run

Your question doesn't have enough information, so I can't help but make assumptions about your code.

• My first assumption is that, based on the signature DataDisplayView_Paint(object s, PaintEventArgs e), your code runs on the GUI thread.

• My second assumption is that your code DOSOMETHING is synchronous.

With that in mind, here's version of your code which guarantees we only run DOSOMETHING if its not already being run:

void DataDisplayView_Paint(object s, PaintEventArgs e)
{
    //DOSOMETHING
}

The GUI thread will only process one message at a time, and your DataDisplayView_Paint method does not exit until DOSOMETHING completes. If you're doing anything with the GUI like drawing to a Graphics object or changing labels, then this code won't get invoked from more than one thread -- and if it does, .NET will throw an exception. In other words, you don't need any synchronization.

---

Let's assume DOSOMETHING runs asyncronously -- now we have an interesting problem, but its very easy to solve, and you don't need any bools.

Essentially, all you're doing is disabling your event handler while DOSOMETHING runs, then re-enabling it. Instead of using a bool, unhook and rehook your event handler as needed:

void DataDisplayView_Paint(object s, PaintEventArgs e)
{
    DataDisplayView.Paint -= DataDisplayView_Paint;
    DoSomethingAsynchronously(); // re-hooks event handler when completed
}

void DoSomethingAsychronously()
{
    ThreadPool.QueueUserWorkItem(() =>
    {
        try
        {
            // DOSOMETHING
        }
        finally
        {
            // may need a lock around this statement
            DataDisplayView.Paint += DataDisplayView_Paint;
        }
    });
}