1

I often use the following pattern to create a cancellable thread:

public class CounterLoop implements Runnable {

    private volatile AtomicBoolean cancelPending = new AtomicBoolean(false);

    @Override
    public void run() {
        while (!cancelPending.get()) {
            //count
        }
    }

    public void cancel() {
        cancelPending.set(true);
    }
}

But I'm not sure that cancelPending MUST be a AtomicBoolean. Can we just use a normal boolean in this case?

user2864740
  • 60,010
  • 15
  • 145
  • 220
Hiep
  • 2,483
  • 1
  • 25
  • 32
  • 1
    rule of the thumb: if you do not use CAS, you do not need AtomicXXX. 2nd rule: if you need test/modify (and modify can go into multiple states) you need CAS. Hence, in your case you don't need AtomicXXX but you can just use Thread.interrupt instead. `interrupt()` is more reaching as well (can cancel blocking and might be able to cancel IO). In your case if you go w/ AtomicXXX, you should make it final, not volatile. – bestsss Feb 14 '13 at 13:08

5 Answers5

2

You can use a volatile boolean instead with no issues.

Note that this only applies in cases much like this where the boolean is only being changed to a specific value (true in this case). If the boolean might be changed to either true or false at any time then you may need an AtomicBoolean to detect and act on race conditions.

However - the pattern you describe has an innate smell. By looping on a boolean (volatile or not) you are likely to find yourself trying to insert some sort of sleep mechanism or having to interrupt your thread.

A much cleaner route is to split up the process into finer steps. I recently posted an answer here covering the options of pausing threads that may be of interest.

Community
  • 1
  • 1
OldCurmudgeon
  • 64,482
  • 16
  • 119
  • 213
2

Using both volatile and AtomicBoolean is unnecessary. If you declare the cancelPending variable as final as follows:

private final AtomicBoolean cancelPending = new AtomicBoolean(false);

the JLS semantics for final fields mean that synchronization (or volatile) will not be needed. All threads will see the correct value for the cancelPending reference. JLS 17.5 states:

"An object is considered to be completely initialized when its constructor finishes. A thread that can only see a reference to an object after that object has been completely initialized is guaranteed to see the correctly initialized values for that object's final fields."

... but there are no such guarantees for normal fields; i.e. not final and not volatile.

You could also just declare cancelPending as a volatile boolean ... since you don't appear to be using the test-and-set capability of AtomicBoolean.

However, if you used a non-volatile boolean you would need to use synchronized to ensure that all threads see an up-to-date copy of the cancelPending flag.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
0

No, you can not. Because if you will change the boolean value from another thread without proper synchronization then this change can be invisible to another threads. You can use valotile boolean in your case to make any modification visible to all threads.

gkuzmin
  • 2,414
  • 17
  • 24
  • actually, you need either `volatile` or `final` ... due to the way that the memory model works. – Stephen C Feb 14 '13 at 14:01
  • I think that Hiep does not want to change the value of `cancelPending ` so the `final` keyword is not needed, but will be more accurate. – gkuzmin Feb 14 '13 at 14:09
  • 2
    It is actually, really needed ... even if nothing modifies the variable. Read my Answer, and the JLS section I've linked to. – Stephen C Feb 14 '13 at 14:12
0

Yes you can. You can either use a non volatile AtomicBoolean (relying on its built in thread safety), or use any other volatile variable.

According to the Java Memory Model (JMM), both options result in a properly synchronized program, where the read and write of the cancelPending variable can't produce a data race.

Eyal Schneider
  • 22,166
  • 5
  • 47
  • 78
  • Actually you need either `volatile` or `final`; see the JLS link in my Answer. – Stephen C Feb 14 '13 at 14:16
  • @StephenC: You are right - in the general case the "final" is required when the variable isn't declared as volatile. However, in practice, usually the thread that interrupts is the same thread where the flag is initialized. In this specific scenario the "happens-before" relation is guaranteed between the writes and reads. – Eyal Schneider Feb 14 '13 at 16:10
0

Using a volatile boolean variable in this context is safe, though some may consider it bad practice. Consult this thread to see why.

Your solution of using an Atomic* variable seems the best option, even though the synchronization may introduce unnecessary overhead in comparison to a volatile variable.

You can also use a critical section 

Object lock = new Object();

@Override
public void run() {
  synchronized (lock) {
    if (cancelPending) {
      return;
    }
  }
}

or a synchronized method.

synchronized public boolean shouldStop() {
  return shouldStop;
}

synchronized public void setStop(boolean stop) {
  shouldStop = stop;
}
Community
  • 1
  • 1
Dariusz
  • 21,561
  • 9
  • 74
  • 114
  • @OldCurmudgeon I disagree. It will work. Synchronized section ensures memory refresh. – Dariusz Feb 14 '13 at 12:58
  • 1
    I stand corrected - I apologise for for my hastiness and withdraw most of my comment (and the downvote). However, I still believe using `synchronized` to flush cache is overkill when `volatile` is available. – OldCurmudgeon Feb 14 '13 at 13:06
  • @StephenC My answer's first sentence means "using a normal boolean is a solution that may work sometimes, but sometimes may not" which in turn should be interpreted as "do not use it". I will edit it though, since this SE is not a place for such remarks. – Dariusz Feb 14 '13 at 13:07
  • @OldCurmudgeon thanks. Using volatile has it's pitfalls, as I stated in my edit. I would not suggest using it, even though in this case it is safe. Doing `var = !var` would not be safe any more. – Dariusz Feb 14 '13 at 13:14