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");}
+}
|
|
|
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) {
|
|
|
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.
|
|
|
|
Java