JDK-6576792 : ThreadPoolExecutor methods leak interrupts when run in pool threads
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 5.0u13,7
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic,solaris_10
  • CPU: generic,sparc
  • Submitted: 2007-07-03
  • Updated: 2011-03-21
  • Resolved: 2007-07-23
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other Other JDK 6 JDK 7
5.0u17-revFixed 5.0u19Fixed 6u10Fixed 7 b17Fixed
Related Reports
Duplicate :  
JDK-6576818 - Apparent LiveConnect changes break applets
Duplicate :  
JDK-6689310 - Backmerge of 6576792 ThreadPoolExecutor#setCorePoolSize interrupts client code
Duplicate :  
JDK-2150756 - Apparent LiveConnect changes break applets
Relates :  
JDK-6450211 - ThreadPoolExecutor.afterExecute sees RuntimeExceptions, but not Errors
Relates :  
JDK-6450200 - ThreadPoolExecutor idling core threads don't terminate when core pool size reduced
Relates :  
JDK-6450205 - ThreadPoolExecutor does not replace throwing threads
Relates :  
JDK-6458339 - ThreadPoolExecutor very slow to shut down for large poolSize
Relates :  
JDK-6454289 - ScheduledThreadPoolExecutor spins while waiting for delayed tasks after shutdown
Relates :  
JDK-6458662 - ThreadPoolExecutor poolSize might shrink below corePoolSize after timeout
Relates :  
JDK-6459119 - Explain how afterExecute can access a submitted job's Throwable
Relates :  
JDK-6450207 - ThreadPoolExecutor doesn't count throwing tasks as "completed"
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
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");} +}
06-07-2007
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.
04-07-2007
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) {
04-07-2007