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

Question

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;
 }
 }
 }
}

• David Harkness: You have return null after waiting for the process which won't compile. This should be return false instead.

Answer 1

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<T>, 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<Boolean> {
 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.

• Lucas AM: process.kill() will abort the process and return immediately. Execute process does implement a Runnable like class. You're saying the correct way would be to remove all those checks and call some kind of interrupt on the thread when I want to cancel it?
• VoiceOfUnreason: I'm saying that if the point of the code is to cancel the process, then you should use the standard idioms for cancellation, instead of rolling your own.

Answer 2

In Java, Locking, Synchronization, and volatility all relate to the state of the Java memory model at a specific point in the code.

Normally, in Java, you cannot really tell what the memory model looks like for each thread. Volatile, synchronization, and other locks change the memory model so that you can make guarantees about it's state at specific code points.

There are books written about this... Java Concurrency in Practice is the most recommended (and I recommend it too).

What is generally accepted is that for any code, you should choose only one of these three tools to manage your code. You should not mix volatile statements with synchronization, and with java.util.concurrent.lock.* structures.

Volatile

volatile is essentially only useful for ensuring that a change in one thread is propagated quickly to another thread. It is not particularly useful for operations that require multiple 'steps' because you still have race conditions between the steps.

In fact, compound operations like x++ are not safe even. Volatile ensures that, at the immediate point the volatile is used, that it is the same in all threads. It does not mean it is the same at the end of the operation in each thread.

In my experience, non-boolean volatile variables are inevitably broken in some way, and boolean volatiles are misused at least half the time.

Admittedly my understanding of the full implications of volatile is incomplete, and is harder to quantify because I first learned Java at a time when volatile often simply did not work. volatile has maybe changed a bit, but, in general, don't use it unless you know what you are doing.

Double-checked locking

 if (!processKilled)
 cancelProcess();

and then cancelProcess contains:

 synchronized (processLock) {
 if (process != null && !processKilled) {
 boolean killed = process.kill();
 if (killed)
 processKilled = true;
 }
 }

What you have here is double-checked locking. Read this paper by Bill Pugh on it.

......

VoiceOfUnreason has also answerd here. My answer is incomplete, but what I would have said is covered well over there

• corsiKa: The most important part about volatile is that it prevents a thread from caching its value. For example while(running) doSomething() may run forever (even when running has been set to false) if it is not volatile.