Stop thread using volatile

Question

I'm facing a problem with stopping some threads I'm creating.

The code

private volatile int status = STOPPED;

@Override
public void run() {

logger.info("Thread with ID : " + id);
status = RUNNING;
while (status == RUNNING) {                
    try {
        execute();      //<-- very intensive 
        status = IDLE;
        Thread.sleep(DYNAMIC_VALUE);
    } catch (Exception e) {
        logger.info("An exception occured in thread with ID : " + id);
        e.printStackTrace();
    } finally {
        if(status == IDLE)
            status = RUNNING;
    }
}

    logger.info("Thread with ID : " + id + " just exited.");
}

@Override
public void stop() {
    status = STOPPED;
    logger.info("Thread with ID: " + id + " is stopped.");
}

Sometimes when I'm calling stop() not all of the threads are stopping. This is never called

logger.info("Thread with ID : " + id + " just exited.");

Instead a logging message inside execute() appears repeatedly, thus it doesn't get stuck somewhere along the way. It's like the value of status doesn't change.

The execute() method is heavyweight. It does a lot of things like updating the database and calling web services.

Do you see anything wrong with the way I'm stopping the threads or should I look deeper? Do I have this aspect covered?

Answer 1

If the threads are still execute()-ing they won't stop since you set their state to IDLE just afterwards, ignoring the previous value. And even adding a check before status = IDLE won't work in all cases.

I suggest to use an AtomicInteger instead of a volatile int and use its compareAndSet method, i.e.:

    while (status.get() == RUNNING) {                
        try {
            execute();      //<-- very intensive 
            if (!status.compareAndSet(RUNNING, IDLE))
                break;
        } catch (Exception e) {
            logger.info("An exception occured in thread with ID : " + id);
            e.printStackTrace();
        } finally {
            try {
                Thread.sleep(DYNAMIC_VALUE);
            } catch (InterruptedException e) {}
            if (!status.compareAndSet(IDLE, RUNNING))
                break;
        }
    }

You should also interrupt the thread after setting its status to STOPPED to interrupt the sleep if applicable.

Answer 2

I suggest using Thread#interrupt() instead of the dual role you have for the status flag now (both to control the thread and to report the status). This is a perfect match for your Thread.sleep() call, it will automatically throw an InterruptedException.

If you do that, you don't have to change anything in the way your status flag is functioning in its reporting role—and you can remove any mention of it in stop().

A bonus of this approach is that you will also be able to add a quick Thread#interrupted check here and there inside the code behind execute, thus making your threads stop even faster; alternatively allowing you to do much more work at once before going to sleep. Eliminating sleep altogether would then become yet another interesting option.

If interrupt isn't an option

If you find a good reason why the interruption mechanism won't work for you, then I would advise something less sophisticated than an AtomicInteger, but just as effective and arguably cleaner: keep the good design idea to let status report status (you could demote it to just boolean and call it active), and create a separate boolean for the running control.

Answer 3

If execute() is running in a thread while you set status=STOPPED, the thread will, on return from execute(), set status=IDLE, and then the finally-block will set status=RUNNING, and your loop loops.

Answer 4

In finally state, you make it RUNNING, it can cause the problem, when you make it STOPPED before it comes to while condition, it can be RUNNING again.

Answer 5

Using your current design, you could check the value of status just before setting it both in the middle of the try block and also in the middle of the finally block.

Cheers