Run update operation in thread only if it isn't already running

Question

I need to do an update operation asynchronously when I receive a notification. The update() method below manipulates instance variables.

    public class UpdateOperation implements Runnable
    {
        private Boolean isInProgress = false;

        @Override
        public void run() {
            try 
            {
                synchronized (isInProgress)
                {
                    isInProgress = true;
                }
                update(); //perform update
                synchronized (isInProgress)
                {
                    isInProgress = false;
                }
            } 
            catch (UpdaterException e) 
            {
                    // deal with it                 
            }
        }

    }

// In another class
private UpdateOperation mCurrentUpdateOperation = new UpdateOperation(); 

public void updateRequired()
{
    synchronized (mCurrentUpdateOperation.isInProgress)
    {
        if (!mCurrentUpdateOperation.isInProgress)
        {
            new Thread(mCurrentUpdateOperation).start();
        }
        else
        {
            // reschedule or silently ignore                
        }
    }   
}

Is this setup sufficient for no two (2) update operations to run concurrently? I believe it is because the first thread to reach the synchronized block will acquire the lock, launch the operation, and release the lock. Then the second (or more) will acquire the lock, see that operation is in progress, reschedule, and release the lock.

Can this setup ever fail?

score 3 · Accepted Answer · edited May 23 '17 at 12:03

3

Is this setup sufficient for no two (2) update operations to run concurrently?

No because of the object you are locking on. You should always synchronize on a non-final object and never on a Boolean. As the value of isInProgress changes (as it is set to true or false), multiple threads will be locking different objects and can enter the mutex block at the same time.

You could instead lock on your UpdateOperation instance if it could be made final. You can always do something like:

 private final Object lockObject = new Object();
 ...
 synchronized (lockObject) {
     ...
 }

Once you lock the object you can check the state of the inProgress which could be a boolean primitive. The synchronize construct syncs all memory. See the Java thread tutorial on synchronization for more information.

Locking on a Boolean is specifically bad because there are only 2 constant object references for them in the entire JVM (unless you do new Boolean(...)). When you say:

isInProgress = true;

you are in effect saying:

isInProgress = Boolean.TRUE;

So all threads in all classes will be locking on the same 2 objects with bizarre results.

For more information see my answer here:

Why is it not a good practice to synchronize on Boolean?

edited May 23 '17 at 12:03

Community

1
1

answered Jan 23 '13 at 19:14

Gray

115,027
24
293
354

(+1) Couldn't have said it better. – NPE Jan 23 '13 at 19:15
With an `Object` lock how do I check the state of the operation? – Sotirios Delimanolis Jan 23 '13 at 19:16
You lock the object and then check the state of `inProgress` @SotiriosDelimanolis. `synchronize` synchronizes _all_ memory so inProgress then can be a primitive boolean if you'd like. See here: http://docs.oracle.com/javase/tutorial/essential/concurrency/sync.html – Gray Jan 23 '13 at 19:21
Couldn't I just lock on my `UpdateOperation` object instead of a new one. – Sotirios Delimanolis Jan 23 '13 at 19:21
1

You could if it never changes. If it was `final` then yes @SotiriosDelimanolis. – Gray Jan 23 '13 at 19:22
Also, what do I do, when I want to change the value of `isInProgress` inside my `run()`? Do I synchronize on `this`? – Sotirios Delimanolis Jan 23 '13 at 19:25
You need to `synchronize` on the _same_ object that the thread synchronizes on. Take a look a the Java tutorial for more details. – Gray Jan 23 '13 at 19:28

Alexei Kaigorodov · Answer 2 · 2019-05-09T15:42:23.183

Another problem of the original solution is that checking and setting the isInProgress variable is in different synchronized statements, which creates time gap. As a result, more than one thread could be started.

Correct solution is:

public class UpdateOperation implements Runnable {
    private boolean inProgress = false;

    public void start() {
       synchronized (this) {
           if (inProgress) {
              return;
           }
           inProgress=true;
       }
       new Thread(this).start();
    }

    @Override
    public void run() {
        try  {
            update(); //perform update
        } catch (UpdaterException e) {
                // deal with it                 
        } finally {
            synchronized (this) {
                inProgress = false;
            }
        }
    }

}

// In another class
private UpdateOperation mCurrentUpdateOperation = new UpdateOperation(); 

public void updateRequired() {
      mCurrentUpdateOperation.start();
}

score 1 · Answer 3 · answered Jan 23 '13 at 19:21

1

Take a look at Executors. They will provide a Threadpool and you can simply add you runnable. Also you may want to use AtomicInteger.

I think this will provide you everything you want, is easier to use.

answered Jan 23 '13 at 19:21

Christian Kuetbach

15,850
5
43
79

1

Using Executors does not help to solve the task. From the task's perspective, there is no difference, if UpdateOperation runs on an executor or on a separate thread. – Alexei Kaigorodov Jan 24 '13 at 04:53
The SingleThreadExecutor will queue you Runnables. So there is only one Operation per time. – Christian Kuetbach Jan 24 '13 at 17:28
When an update is already running, the OP wants to either "reschedule or silently ignore". With an Executor you reschedule the subsequent operation (it is placed in the queue and executed after), but you cannot get the "silently ignore" behaviour. – Andrew Spencer May 08 '19 at 13:00

Run update operation in thread only if it isn't already running

3 Answers3