Controlling an instance's state with AtomicBoolean

Question

I need to ensure that a particular start and stop code is executed only once per instance lifecycle, and that the instance cannot be "restarted". Is the following code adequate for a scenario where multiple threads may be acting upon the instance?

public final class MyRunnable {
    private final AtomicBoolean active = new AtomicBoolean(false);
    private final AtomicBoolean closed = new AtomicBoolean(false);

    public void start() {
      if (closed.get()) {
        throw new IllegalStateException("Already closed!");
      }
      if (active.get()) {
        throw new IllegalStateException("Already running!");
      }

      active.set(true);

      // My one-time start code.

      // My runnable code.
    }

    public void stop() {
      if (closed.get()) {
        throw new IllegalStateException("Already stopped!");
      }
      if (!active.get()) {
        throw new IllegalStateException("Stopping or already stopped!");
      }

      active.set(false);

      // My one-time stop code.

      closed.set(true);
    }
}

Answer 1

I would go with a single 3-valued status for two reasons.

Firstly, out of the 4 possible values of the active,closed "tuple" only 3 make sense, setting both to true results in a (possibly benign, but nevertheless) invalid state. You may dismiss it as pure pedantry, but a clear design often brings other benefits.

This leads us neatly to the second, scarier reason:

 active.set(false);
 // <-- what if someone calls start() here?
 closed.set(true); //I assume you wanted to set it to true

As you can see from my comment, you've got a vulnerable spot there, someone could conceivably call start() after you've set active to false but before you set closed to true.

Now you may just say "okay, let's swap the two then and set closed first", but then you have to explain why the two would definitely not be reordered by the JVM. And you'll end up with potentially both flags set to true, resulting in the "invalid state" outlined above.

There is another, separate problem here: the pattern you follow is to call get() to check the value and then set() it to something else later. As PetrosP pointed it out, this isn't an atomic operation, you can call start() a 1000 times with all of them seeing active as false. You need to use compareAndSet instead, which is atomic (this is the whole point of the Atomic* classes), and thus guarantees that only one thread can ever advance the status flag.

So let's combine the two, using a single 3-valued status (I've used AtomicInteger for simplicity, but you can use AtomicReference and a true enum) and compareAndSet():

public final class MyRunnable {
    private static final int READY_TO_START = 0;
    private static final int ACTIVE = 1;
    private static final int STOPPED = 2;
    private final AtomicInteger status = new AtomicInteger(READY_TO_START);

    public void start() {
      if (!status.compareAndSet(READY_TO_START, ACTIVE)) {
        throw new IllegalStateException("Already started");
      }

      // My one-time start code.
    }

    public void stop() {
        if (!status.compareAndSet(ACTIVE, STOPPED)) {
            throw new IllegalStateException("Can't stop, either not started or already stopped");
        }

      // My one-time stop code.
    }
}