Skip to end of metadata
Go to start of metadata

Programs may submit only tasks that support interruption using Thread.interrupt() to thread pools that require the ability to shut down the thread pool or to cancel individual tasks within the pool. Programs must not submit tasks that lack interruption support to such thread pools. According to the Java API [API 2014], the java.util.concurrent.ExecutorService.shutdownNow() method

attempts to stop all actively executing tasks, halts the processing of waiting tasks, and returns a list of the tasks that were awaiting execution....

There are no guarantees beyond best-effort attempts to stop processing actively executing tasks. For example, typical implementations will cancel via Thread.interrupt(), so any task that fails to respond to interrupts may never terminate.

Noncompliant Code Example (Shutting Down Thread Pools)

This noncompliant code example submits the SocketReader class as a task to the thread pool declared in PoolService:

public final class SocketReader implements Runnable { // Thread-safe class
  private final Socket socket;
  private final BufferedReader in;
  private final Object lock = new Object();

  public SocketReader(String host, int port) throws IOException {
    this.socket = new Socket(host, port);
    this.in = new BufferedReader(
        new InputStreamReader(this.socket.getInputStream())
    );
  }

  // Only one thread can use the socket at a particular time
  @Override public void run() {
    try {
      synchronized (lock) {
        readData();
      }
    } catch (IOException ie) {
      // Forward to handler
    }
  }

  public void readData() throws IOException {
    String string;
    try {
      while ((string = in.readLine()) != null) {
        // Blocks until end of stream (null)
      }
    } finally {
      shutdown();
    }
  }

  public void shutdown() throws IOException {
    socket.close();
  }
}

public final class PoolService {
  private final ExecutorService pool;

  public PoolService(int poolSize) {
    pool = Executors.newFixedThreadPool(poolSize);
  }

  public void doSomething() throws InterruptedException, IOException {
    pool.submit(new SocketReader("somehost", 8080));
    // ...
    List<Runnable> awaitingTasks = pool.shutdownNow();
  }

  public static void main(String[] args) 
                          throws InterruptedException, IOException {
    PoolService service = new PoolService(5);
    service.doSomething();
  }
}

The shutdownNow() method may fail to shut down the thread pool because the task lacks support for interruption using the Thread.interrupt() method and because the shutdown() method must wait until all executing tasks have finished.

Similarly, tasks that use some mechanism other than Thread.interrupted() to determine when to shut down will be unresponsive to shutdown() and shutdownNow(). For instance, tasks that check a volatile flag to determine whether it is safe to shutdown are unresponsive to these methods. THI05-J. Do not use Thread.stop() to terminate threads provides more information on using a flag to terminate threads.

Compliant Solution (Submit Interruptible Tasks)

This compliant solution defines an interruptible version of the SocketReader class, which is instantiated and submitted to the thread pool:

public final class SocketReader implements Runnable {
  private final SocketChannel sc;
  private final Object lock = new Object();

  public SocketReader(String host, int port) throws IOException {
    sc = SocketChannel.open(new InetSocketAddress(host, port));
  }

  @Override public void run() {
    ByteBuffer buf = ByteBuffer.allocate(1024);
    try {
      synchronized (lock) {
        while (!Thread.interrupted()) {
          sc.read(buf);
          // ...
        }
      }
    } catch (IOException ie) {
      // Forward to handler
    }
  }
}

public final class PoolService {
  // ...
}

Exceptions

TPS02-J-EX0: Short-running tasks that execute without blocking are exempt from this rule.

Risk Assessment

Submitting tasks that are uninterruptible may prevent a thread pool from shutting down and consequently may cause DoS.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

TPS02-J

Low

Probable

Medium

P4

L3

Bibliography

[API 2014]

Interface ExecutorService

[Goetz 2006a]

Chapter 7, "Cancellation and Shutdown"

 


6 Comments

    • There is a fundamental problem with running interruptible code, and I think it is essentially the halting problem. Essentially, you can't claim a task to be interruptible if it runs any routines which are non-interruptible (and may take a long time or block). This is a consequence of Java's fundamental design decision to not allow threads to be preemptively stopped (notice that Thread.stop() is deprecated).
    • The code sample provided in the intro is useful for worker threads, but it is unnecessary if you are just submitting tasks to a thread pool. The thread pool is supposed to implement this code sample and so you don't have to worry about it. It is a useful illustration, however.
    • The NCCE & CS provided are technically sound. But they both depend on code from CON24-J, and the differences between the NCCE & CS are confined to which code from CON24 you use (both are considered compliant there). I'm convinced this is a good technical solution, but I'm not sure if this rule should lean so heavily on CON24-J. We usually don't have rules that depend on other rules for comprehension of their code samples. Wouldn't a smaller NCCE & CS be easier to understand (for those who haven't studied CON24-J)?
      • Are you suggesting that we add exceptions to the guideline to the effect of "if there is no equivalent API that supports interruption, document that the task is not interruptible and carry on"?
      • Worker threads mentioned here are our usual runnable and callable tasks. I think the point is that a non-interruptible (runnable/callable) task should be wrapped in an interruptible worker thread and then submitted to the pool.
      • Didn't want to replicate the same class because it is quite long. A smaller NCE/CS might be feasible if you can think of one that compiles and works. We should probably discuss this one offline.
  1. In the compliant solution why is there a lock variable?  The lock is only to guard access to the socket channel, so shouldn't you just synchronize on sc.

     

    1. Synchronizing on sc would work. We prefer to lock on a private final object that serves no other purpose but to be a lock is the best solution. See LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code for more info.

      1. Wow, I never thought of that attack vector.  Thanks!

        So technically in this particular code example it could still be locked safely on sc, except that if someone modifies the class later on and leaks an sc (directly or indirectly) then it would be vulnerable.

        I guess I've always dealt with this by having the rule that locked objects should never leak from the scope of the class that is performing the locking (as then the user of the leaked object needs to understand the locking mechanism of the resource).  Is this alternate rule vulnerable? LCK00-J is probably better, as my alternative doesn't allow for synchronized methods (implicit class or object reference which can't be stopped from leaking).

        1. Correct...locking on sc is thread-safe...for now (smile)

          I suppose if you can guarantee the scope of your object (eg it is private to another class and never escapes and will never escape during the maintenance of the code), then locking on the object is OK.  It's also more convenient, as Java makes it easy to synchronize on the 'this' object.  Personally I have no control over code once I leave the company, so I can't make any such guarantees :S