United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-6576792 ThreadPoolExecutor methods leak interrupts when run in pool threads
JDK-6576792 : ThreadPoolExecutor methods leak interrupts when run in pool threads

Details
Type:
Bug
Submit Date:
2007-07-03
Status:
Resolved
Updated Date:
2011-03-21
Project Name:
JDK
Resolved Date:
2007-07-23
Component:
core-libs
OS:
generic,solaris_10
Sub-Component:
java.util.concurrent
CPU:
sparc,generic
Priority:
P2
Resolution:
Fixed
Affected Versions:
5.0u13,7
Fixed Versions:

Related Reports
Backport:
Backport:
Backport:
Duplicate:
Duplicate:
Duplicate:
Relates:
Relates:
Relates:
Relates:
Relates:
Relates:
Relates:
Relates:

Sub Tasks

Description
The set of fixes to the ThreadPoolExecutor incorporated in JDK 7 build 08 have introduced a regression where the interruptions used to shut down idle threads can reach user code.

This regression was discovered in the context of the Iris demonstration shown at this year's JavaOne, http://swinglabs.org/iris/ . It runs successfully with JDK 7 build 07 but not with build 08. To reproduce the problem, run Internet Explorer 6 or 7 on Windows (it appears that a plugin bug, being investigated separately, is currently causing it to fail to work on Firefox) and navigate to http://swinglabs.org/iris/ . Enter a Flickr account name such as "kenneth russell", "jasper potts", or "romainguy". When the photosets appear, click on one of the thumbnails in the leftmost column. When the bug is present, the thumbnails of the photos in the photoset will fail to load into the rightmost pane, instead producing an InterruptedException from Iris's LoadPhotosTask.

Attached is the output of the Java Console from an instrumented JDK which calls Thread.dumpStack() inside Thread.interrupt(). It shows that the only calls to Thread.interrupt() are coming from ThreadPoolExecutor.setCorePoolSize(), but that the interrupted state is propagating out to and affecting user code, which should not happen and did not happen before JDK 7 build 08.

                                    

Comments
SUGGESTED FIX

Based on comments from ###@###.###, this suggested fix solves the problem:

*** /tmp/geta2256	Tue Jul  3 21:40:02 2007
--- ThreadPoolExecutor.java	Tue Jul  3 21:35:46 2007
***************
*** 726,736 ****
       */
      private void interruptIdleWorkers(boolean onlyOne) {
          final ReentrantLock mainLock = this.mainLock;
          mainLock.lock();
          try {
  	    for (Worker w : workers) {
                  Thread t = w.thread;
!                 if (!t.isInterrupted() && w.tryLock()) {
                      try {
                          t.interrupt();
                      } catch (SecurityException ignore) {
--- 726,738 ----
       */
      private void interruptIdleWorkers(boolean onlyOne) {
          final ReentrantLock mainLock = this.mainLock;
+         final Thread current = Thread.currentThread();
          mainLock.lock();
          try {
  	    for (Worker w : workers) {
                  Thread t = w.thread;
!                 // We should not interrupt ourselves
!                 if (t != current && !t.isInterrupted() && w.tryLock()) {
                      try {
                          t.interrupt();
                      } catch (SecurityException ignore) {
                                     
2007-07-04
EVALUATION

Each Worker is a ReentrantLock, so it is possible for a worker to acquire
this lock while non-idle, which was not anticipated by the implementation.

This is one case where a non-reentrant lock is just what we need.
Perhaps we should add a serious Mutex class to the JDK, instead of just
using it as a demo?

This bug is pre-existing.  However, changes for
6450200: ThreadPoolExecutor idling core threads don't terminate when core pool size reduced
appear to have made this bug more likely to be triggered because
interrupts are used more aggressively to terminate idle threads.

Here is a test case demonstrating the existing bug that fails with all current
versions of the JDK:

-----------------------------

import java.util.concurrent.*;

public class Bug {
    void test(String[] args) throws Throwable {
	final int n = 100;
	final ThreadPoolExecutor pool =
	    new ThreadPoolExecutor(n, n, 1L, TimeUnit.NANOSECONDS,
				   new SynchronousQueue<Runnable>());
	final CountDownLatch startingGate = new CountDownLatch(n);
	final CountDownLatch finishLine = new CountDownLatch(n);
	equal(pool.getCorePoolSize(), n);
	equal(pool.getPoolSize(), 0);
	for (int i = 0; i < n; i++)
	    pool.execute(new Runnable() { public void run() {
		try {
		    startingGate.countDown();
		    startingGate.await();
		    equal(pool.getPoolSize(), n);
		    pool.setCorePoolSize(n);
		    pool.setCorePoolSize(1);
		    check(! Thread.interrupted());
		    equal(pool.getPoolSize(), n);
		    finishLine.countDown();
		    finishLine.await();
		    check(! Thread.interrupted());
		} catch (Throwable t) { unexpected(t); }}});
	finishLine.await();
	pool.shutdown();
	check(pool.awaitTermination(1L, TimeUnit.DAYS));
    }

    //--------------------- Infrastructure ---------------------------
    volatile int passed = 0, failed = 0;
    void pass() {passed++;}
    void fail() {failed++; Thread.dumpStack();}
    void fail(String msg) {System.err.println(msg); fail();}
    void unexpected(Throwable t) {failed++; t.printStackTrace();}
    void check(boolean cond) {if (cond) pass(); else fail();}
    void equal(Object x, Object y) {
	if (x == null ? y == null : x.equals(y)) pass();
	else fail(x + " not equal to " + y);}
    public static void main(String[] args) throws Throwable {
	new Bug().instanceMain(args);}
    void instanceMain(String[] args) throws Throwable {
	try {test(args);} catch (Throwable t) {unexpected(t);}
	System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
	if (failed > 0) throw new AssertionError("Some tests failed");}
}
-----------------------------

Here is a fix:

--- /tmp/geta8847	2007-07-04 01:29:30.584308200 -0700
+++ ThreadPoolExecutor.java	2007-07-03 23:31:54.768968000 -0700
@@ -721,20 +721,22 @@
      * workers since shutdown began will also eventually exit.
      * To guarantee eventual termination, it suffices to always
      * interrupt only one idle worker, but shutdown() interrupts all
      * idle workers so that redundant workers exit promptly, not
      * waiting for a straggler task to finish.
      */
     private void interruptIdleWorkers(boolean onlyOne) {
-        final ReentrantLock mainLock = this.mainLock;
+	final Thread current = Thread.currentThread();
+	final ReentrantLock mainLock = this.mainLock;
         mainLock.lock();
         try {
 	    for (Worker w : workers) {
                 Thread t = w.thread;
-                if (!t.isInterrupted() && w.tryLock()) {
+		// We should not interrupt ourselves
+		if (t != current && !t.isInterrupted() && w.tryLock()) {
                     try {
                         t.interrupt();
                     } catch (SecurityException ignore) {
                     } finally {
                         w.unlock();
                     }
                 }

but a more efficient fix might be to make Worker a simple non-reentrant lock.
                                     
2007-07-04
EVALUATION

We eventually did decide to make Worker a non-reentrant lock class.

--- old/src/share/classes/java/util/concurrent/ThreadPoolExecutor.java	2007-07-05 17:22:18.427447000 -0700
+++ new/src/share/classes/java/util/concurrent/ThreadPoolExecutor.java	2007-07-05 17:22:18.249827000 -0700
@@ -562,14 +562,20 @@
 
     /**
      * Class Worker mainly maintains interrupt control state for
-     * threads running tasks, along with other minor bookkeeping. This
-     * class opportunistically extends ReentrantLock to simplify
-     * acquiring and releasing a lock surrounding each task execution.
-     * This protects against interrupts that are intended to wake up a
-     * worker thread waiting for a task from instead interrupting a
-     * task being run.
-     */
-    private final class Worker extends ReentrantLock implements Runnable {
+     * threads running tasks, along with other minor bookkeeping.
+     * This class opportunistically extends AbstractQueuedSynchronizer
+     * to simplify acquiring and releasing a lock surrounding each
+     * task execution.  This protects against interrupts that are
+     * intended to wake up a worker thread waiting for a task from
+     * instead interrupting a task being run.  We implement a simple
+     * non-reentrant mutual exclusion lock rather than use ReentrantLock
+     * because we do not want worker tasks to be able to reacquire the
+     * lock when they invoke pool control methods like setCorePoolSize.
+     */
+    private final class Worker
+	extends AbstractQueuedSynchronizer
+	implements Runnable
+    {
 	/**
 	 * This class will never be serialized, but we provide a
 	 * serialVersionUID to suppress a javac warning.
@@ -596,6 +602,34 @@
         public void run() {
             runWorker(this);
         }
+
+	// Lock methods
+	//
+	// The value 0 represents the unlocked state.
+	// The value 1 represents the locked state.
+
+	protected boolean isHeldExclusively() {
+	    return getState() == 1;
+	}
+
+	protected boolean tryAcquire(int unused) {
+	    if (compareAndSetState(0, 1)) {
+		setExclusiveOwnerThread(Thread.currentThread());
+		return true;
+	    }
+	    return false;
+	}
+
+	protected boolean tryRelease(int unused) {
+	    setExclusiveOwnerThread(null);
+	    setState(0);
+	    return true;
+	}
+
+	public void lock()        { acquire(1); }
+	public boolean tryLock()  { return tryAcquire(1); }
+	public void unlock()      { release(1); }
+	public boolean isLocked() { return isHeldExclusively(); }
     }
 
     /*
@@ -725,12 +759,12 @@
      * waiting for a straggler task to finish.
      */
     private void interruptIdleWorkers(boolean onlyOne) {
-        final ReentrantLock mainLock = this.mainLock;
+	final ReentrantLock mainLock = this.mainLock;
         mainLock.lock();
         try {
 	    for (Worker w : workers) {
                 Thread t = w.thread;
-                if (!t.isInterrupted() && w.tryLock()) {
+		if (!t.isInterrupted() && w.tryLock()) {
                     try {
                         t.interrupt();
                     } catch (SecurityException ignore) {
--- /dev/null	2007-07-05 17:22:20.000000000 -0700
+++ new/test/java/util/concurrent/ThreadPoolExecutor/SelfInterrupt.java	2007-07-05 17:22:20.031280000 -0700
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2007 Sun Microsystems, Inc.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/*
+ * @test 1.1 07/07/05
+ * @bug 6576792
+ * @summary non-idle worker threads should not be interrupted
+ */
+
+import java.util.concurrent.*;
+
+public class SelfInterrupt {
+    void test(String[] args) throws Throwable {
+	final int n = 100;
+	final ThreadPoolExecutor pool =
+	    new ThreadPoolExecutor(n, n, 1L, TimeUnit.NANOSECONDS,
+				   new SynchronousQueue<Runnable>());
+	final CountDownLatch startingGate = new CountDownLatch(n);
+	final CountDownLatch finishLine = new CountDownLatch(n);
+	equal(pool.getCorePoolSize(), n);
+	equal(pool.getPoolSize(), 0);
+	for (int i = 0; i < n; i++)
+	    pool.execute(new Runnable() { public void run() {
+		try {
+		    startingGate.countDown();
+		    startingGate.await();
+		    equal(pool.getPoolSize(), n);
+		    pool.setCorePoolSize(n);
+		    pool.setCorePoolSize(1);
+		    check(! Thread.interrupted());
+		    equal(pool.getPoolSize(), n);
+		    finishLine.countDown();
+		    finishLine.await();
+		    check(! Thread.interrupted());
+		} catch (Throwable t) { unexpected(t); }}});
+	finishLine.await();
+	pool.shutdown();
+	check(pool.awaitTermination(1000L, TimeUnit.SECONDS));
+    }
+
+    //--------------------- Infrastructure ---------------------------
+    volatile int passed = 0, failed = 0;
+    void pass() {passed++;}
+    void fail() {failed++; Thread.dumpStack();}
+    void fail(String msg) {System.err.println(msg); fail();}
+    void unexpected(Throwable t) {failed++; t.printStackTrace();}
+    void check(boolean cond) {if (cond) pass(); else fail();}
+    void equal(Object x, Object y) {
+	if (x == null ? y == null : x.equals(y)) pass();
+	else fail(x + " not equal to " + y);}
+    public static void main(String[] args) throws Throwable {
+	new SelfInterrupt().instanceMain(args);}
+    void instanceMain(String[] args) throws Throwable {
+	try {test(args);} catch (Throwable t) {unexpected(t);}
+	System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
+	if (failed > 0) throw new AssertionError("Some tests failed");}
+}
                                     
2007-07-06



Hardware and Software, Engineered to Work Together