Using synchronized blocks and volatile variables to ask a Java thread to terminate gracefully

Edit: Just to clarify, this isn't my original code, I've simplified it to make it smaller/easier to understand what I'm trying to do.

It's my first time working with threads in Java, and I was wondering if I'm synchronizing correctly. I did some quick research on locks, synchronized and volatile, but I'm still not sure where/how to use them. Basically I have a thread that executes some process, and I can cancel it from an outside thread (calling cancelProcess()). Between each step I check if the process should be canceled.

public final class ExecuteProcess {
    private volatile boolean processShouldCancel;
    private volatile boolean processKilled;
    private final Object processLock = new Object();
    private Process process;

    public boolean run() {
        initialize();

        if (processShouldCancel) {
            showCanceledMsg();
            return false;
        }

        synchronized (processLock) {
            process = new Process();
            process.Start();
        }

        if (processShouldCancel) {
            if (!processKilled)
                cancelProcess();

            showCanceledMsg();
            return false;
        }

        process.waitForResponse();

        if (processKilled) {
            showCanceledMsg();
            return false;
        }

        if (process.finished()) {
            process.getResult();
            return true;
        }

        return false;
    }

    public void cancelProcess() {
        processShouldCancel = true;
        synchronized (processLock) {
            if (process != null && !processKilled) {
                boolean killed = process.kill();

                if (killed)
                    processKilled = true;
            }
        }
    }
}

Find a copy of Java Concurrency in Practice, it's got very good chapter on Cancellation and Shutdown.

You've made cancelProcess() private, so your outside threads aren't going to do anything to prevent process.waitForResponse() from blocking. That's no good - let's make it public.

In addition to that, trying to cancel a running process by setting a volatile variable is an anti-pattern. For your specific example, consider what happens if the external thread tries to cancel while process.waitForResponse() is running. It doesn't do much good, does it -- ExecuteProcess can't check processKilled until the waitForReponse returns, and by then there is no point, you've already been forced to wait.


Interruption is usually the most sensible way to implement cancellation.

As noted above, you can write an entire chapter on that problem; I'll spare us all that. What that quotation does mean, though, is that task cancellation is a lousy example for learning about volatile variables.

Code Review

There's already a standard interface called Runnable; it's appropriate to use it here. The runnable method returns a void though, so you should consider other ways to signal success and failure. An alternative to Runnable would be a Callable, which supports returning values.


The Callable interface is similar to Runnable, in that both are designed for classes whose instances are potentially executed by another thread. A Runnable, however, does not return a result and cannot throw a checked exception.

Creating a process is a separate responsibility from running a process. You should probably inject the process to be run, via the constructor. The class might then look like:

public class ExecuteProcess implements Callable {
    private final Process process;

    public ExecuteProcess(Process process) {
        this.process = process;
    }

    Boolean call () {
        ...
    }
}

The advantage of Runnable and Callable: you can now take advantage of ExecutorService.submit(), which returns a Future. Future.cancel(), under the covers, arranges for Thread.interrupt() to be called the right way.

Having done that, you can now clean up ExecuteProcess. Instead of checking synchronized state, your process code is responsible for checking to see if the thread has been interrupted...

Executor Service and Future are part of the java.util.concurrent library; a library that gets the little details right so that you don't have to. You can find sources on line if you want to look at the details for yourself: FutureTask.Sync.innerCancel might be a good starting point.

Also, it's really poor form to return null in a function that promises a boolean.

StackExchange Code Review Q#52454, answer score: 11