According to the Java Language Specification, §17.3, "Sleep and Yield" [JLS 2013],

It is important to note that neither Thread.sleep nor Thread.yield have any synchronization semantics. In particular, the compiler does not have to flush writes cached in registers out to shared memory before a call to Thread.sleep or Thread.yield, nor does the compiler have to reload values cached in registers after a call to Thread.sleep or Thread.yield.

Code that bases its concurrency safety on thread suspension or yields to processes that

  • Flush cached registers,
  • Reload any values,
  • Or provide any happens-before relationships when execution resumes,

is incorrect and is consequently disallowed. Programs must ensure that communication between threads has proper synchronization, happens-before, and safe publication semantics.

Noncompliant Code Example (sleep())

This noncompliant code attempts to use the nonvolatile primitive Boolean member done as a flag to terminate execution of a thread. A separate thread sets done to true by calling the shutdown() method.

final class ControlledStop implements Runnable {
  private boolean done = false;

  @Override public void run() {
    while (!done) {
      try {
        Thread.sleep(1000);
      } catch (InterruptedException e) {
        // Reset interrupted status
        Thread.currentThread().interrupt(); }
    }
  }

  public void shutdown() {
    this.done = true;
  }
}

The compiler, in this case, is free to read the field this.done once and to reuse the cached value in each execution of the loop. Consequently, the while loop might never terminate, even when another thread calls the shutdown() method to change the value of this.done [JLS 2013]. This error could have resulted from the programmer incorrectly assuming that the call to Thread.sleep() causes cached values to be reloaded.

Compliant Solution (Volatile Flag)

This compliant solution declares the flag field volatile to ensure that updates to its value are made visible across multiple threads:

final class ControlledStop implements Runnable {
  private volatile boolean done = false;

  @Override public void run() {
    //...
  }

  // ...
}

The volatile keyword establishes a happens-before relationship between this thread and any other thread that sets done.

Compliant Solution (Thread.interrupt())

A better solution for methods that call sleep() is to use thread interruption, which causes the sleeping thread to wake immediately and handle the interruption.

final class ControlledStop implements Runnable {

  @Override public void run() {
    // Record current thread, so others can interrupt it
    myThread = currentThread();
    while (!Thread.interrupted()) {
      try {
        Thread.sleep(1000);
      } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
      }
    }
  }

  public void shutdown(Thread th) {
    th.interrupt();
  }
}

Note that the interrupting thread must know which thread to interrupt; logic for tracking this relationship has been omitted from this solution.

Noncompliant Code Example (getState())

This noncompliant code example contains a doSomething() method that starts a thread. The thread supports interruption by checking a flag and waits until notified. The stop() method checks to see whether the thread is blocked on the wait; if so, it sets the flag to true and notifies the thread so that the thread can terminate.

public class Waiter {
  private Thread thread;
  private boolean flag;
  private final Object lock = new Object();

  public void doSomething() {
    thread = new Thread(new Runnable() {
      @Override public void run() {
        synchronized(lock) {
          while (!flag) {
            try {
              lock.wait();
              // ...
            } catch (InterruptedException e) {
              // Forward to handler
            }
          }
        }
      }
    });
    thread.start();
  }

  public boolean stop() {
    if (thread != null) {
      if (thread.getState() == Thread.State.WAITING) {
        synchronized (lock) {
          flag = true;
          lock.notifyAll();
        }
        return true;
      }
    }
    return false;
  }
}

Unfortunately, the stop() method incorrectly uses the Thread.getState() method to check whether the thread is blocked and has not terminated before delivering the notification. Using the Thread.getState() method for synchronization control, such as checking whether a thread is blocked on a wait, is inappropriate. Java Virtual Machines (JVMs) are permitted to implement blocking using spin-waiting; consequently, a thread can be blocked without entering the WAITING or TIMED_WAITING state [Goetz 2006]. Because the thread may never enter the WAITING state, the stop() method might fail to terminate the thread.

If doSomething() and stop() are called from different threads, the stop() method could fail to see the initialized thread, even though doSomething() was called earlier, unless there is a happens-before relationship between the two calls. If the two methods are invoked by the same thread, they automatically have a happens-before relationship and consequently cannot encounter this problem.

Compliant Solution

This compliant solution removes the check for determining whether the thread is in the WAITING state. This check is unnecessary because invoking notifyAll() affects only threads that are blocked on an invocation of wait():

public class Waiter {
  // ...
  private Thread thread;
  private volatile boolean flag;
  private final Object lock = new Object();

  public boolean stop() {
    if (thread != null) {
      synchronized (lock) {
        flag = true;
        lock.notifyAll();
      }
      return true;
    }
    return false;
  }
}

Applicability

Relying on the Thread class's sleep(), yield(), and getState() methods for synchronization control can cause unexpected behavior.

Bibliography

 


9 Comments

  1. The last NCCE/CS examples aren't quite correct.  the "thread" member isn't volatile, so a caller of stop() may not see it initialized.  as a minor secondary note, the "flag" member doesn't technically need to be volatile because the synchronization provides the necessary guarantees.

    1. I added a paragraph about the possibility of the initialized thread not being visible. (I believe the example assumed that doSomething() and stop() were invoked by the same thread, in which case this is not a problem.)

      You're also right about the flag not needing to be volatile, I took that out.

      1. maybe it would be easier to just make the "thread" member volatile instead of having the extra explanation (since that isn't really the point of this example anyway).

  2. Moved setting the flag inside the synchronized block in stop().  I believe the change is not strictly necessary because we'll have the right happens-before relationships anyway, but this way it's clear that flag (which is accessed in both threads) is protected by lock, and consequently it's obvious that the synchronization is good.

  3. After approving this rule, I discovered that it lives in The Void. That's no place for a fine upstanding rule like this one. I moved it out.

  4. Fond the following comment in the markup:

    This does not talk about invoking getState() and comparing with TERMINATE, RUNNABLE and other states. Should we?

    I guess we should add this. I am moving on but request someone to add some lines for ^^^.

  5. Although this rule is correct, it really should have some reference to using locks as defined in java.util.concurrent. In general using java.util.concurrent should be preferred over rolling your own. In many ways this is similar advice as "never roll your own crypto" (unless you're an expert or scholar).

    1. The problem of not providing any thread-safety on a shared variable occurs often in our Java rules and guidelines. Here, for example. The 1st NCCE could be solved with java.concurrent.Lock objects, or with the objects' monitors, or with atomic objects. This rule was one case where we did not showcase every possible way to address the concurrency. Locks or atomics would be correct, but volatile works here and is the most lightweight.

      1. Fair enough, David. As long as the proposed solution is secure there's not much I can complain about. You might consider including a general statement to using java.util.concurrent. Otherwise users will just have to read the comments (smile) Thanks (in general) for taking my comments seriously and performing the necessary actions to update the recommendations in such timely fashion!