Prevent concurrent method execution

Question

I have a class which has a start() and stop() method and want to do the following:

1 One of the methods must not be called while the other one is running, e.g. when start() is called then stop() must not be called at the same time.
2a A call to either of the methods while it is already running should either be skipped immediately,
2b or they should return the same time the initial call (which owns the lock) returns.

I thought that the first requirement could be achieved by adding a synchronized block. The requirement 2a is probably a good fit for an AtomicBoolean. However, to me this looks more complicated than it should be. Are there other possibilities? Is there maybe a class that fulfills both requirements in one go? Don't really know how 2b could be achieved.

When I just re-read my example code I realized that the synchronized probably needs to be before the isStarted check. However, then I could not return immediately when a call is already inside the method. On the other hand, with the code given as below, I cannot prevent that one thread compares and sets isStarted to true and then gets suspended and another thread in stop() does the stop logic although the start() logic has not been completed.

Example code

    private final AtomicBoolean isStarted = new AtomicBoolean(false);
    private final Object lock = new Object();

    public void start() {
        if (isStarted.compareAndSet(false, true)) {
            synchronized(lock) {
                // start logic
            }
        } else {
            log.warn("Ignored attempt to start()");
        }
    }

    public void stop() {
        if (isStarted.compareAndSet(true, false)) {
            synchronized(lock) {
                // stop logic
            }
        } else {
            log.warn("Ignored attempt to stop()");
        }        
    }

Answer 1

Your code can implement a kind of double checking like this:

private final Object lock = new Object();
private volatile boolean isStarted = false;

public void start() {
    if (isStarted) {
        return;
    }
    synchronized(lock) {
        if (isStarted) {
            return;
        }
        isStarted = true;

        // ...do start...
    }
}

public void stop() {
    if (!isStarted) {
        return;
    }
    synchronized(lock) {
        if (!isStarted) {
            return;
        }
        isStarted = false;

        // ...do stop...
    }
}

This should match all of your conditions 1, 2a, 2b. If you expose isStarted, you could modify the flag AFTER, not BEFORE the action (do start/do stop) finished, but in this case 2b will happen a bit frequently.

But, in the real life, I'd prefer to use messaging between threads rather than locking which can lead to deadlocks easily (for example, you notify some listeners under the lock and the listeners use their own locks).

A simple BlockingQueue may be such messaging queue. One single thread consumes all the commands from the queue and executes them one by one (in its run() method) according to its current state which isn't a shared one now. Another threads put Start/Stop commands to the queue.

Answer 2

Let's imagine this scenario:

1. Thread A enters 'start'.
2. Thread A is busy, trying to dig through the 'start' logic. It is taking a while.
3. Whilst A is still busy, thus the status of your code is 'starting in progress', but not yet 'I have started', thread B too calls start.

What you said is that you're okay with B returning immediately.

I question whether you really mean that; that would mean that code in B invokes 'start', and then this invocation returns, but the object isn't in the 'started' state yet. Surely you intend for a call to start() to return normally only once the object has actually been placed in the 'running' state, and return via exception if that is not possible.

Given that I interpreted your needs correctly, forget the boolean; all you need is synchronized. Remember, locks are part of your API, and we don't have public fields as a rule in java, so you shouldn't have public locks unless you document extensively (and consider the way your code interacts with the lock as part of the public API that you cannot modify without breaking backwards compatibility - not usually a promise you want to hand out casually, as that's quite a pair of handcuffs for future updates), so, your idea to make a private lock field is great. However, we already have a private unique instance, which we can re-use for the locks:

    private final AtomicBoolean isStarted = new AtomicBoolean(false);

    public void start() {
        synchronized (isStarted) {
            if (isStarted.compareAndSet(false, true)) startLogic();
        }
    }

    public void stop() {
        synchronized (isStarted) {
            if (isStarted.compareAndSet(true, false)) stopLogic();
        }
    }

    public boolean isStarted() {
        return isStarted.get();
    }

    private void startLogic() { ... }
    private void stopLogic() { ... }
}

This code will ensure that invoking 'start();' will guarantee that the code was in 'started' state when it returns normally, or at least, it was (the one exception is if some other thread was waiting to stop it and did so immediately after the thread was started; presumably, as weird as that is, whichever state of affairs caused stop() to run wanted this to happen).

Furthermore, if you want the locking behaviour to be part of the API of this class, you can do so. For example, you could add:

/** Runs the provided runnable such that the service is running throughout.
 * Will start the service if neccessary and will block attempts to stop
 * the service whilst running. Restores the state afterwards.
 */
public void run(Runnable r) {
    synchronized (isStarted) {
        boolean targetState = isStarted.get();
        start(); // this is no problem; locks are re-entrant in java.
        r.run();
        if (!targetState) stop();
    }
}