Hey Thanks Chris, Awesome to see some contribution.
I committed something very similar last night, it could use some
refactoring too, the only difference from what you have here is the
TaskThread creates the thread in its constructor, but it doesn't start
the thread in the constructor. I added an additional method
TaskTread.start() to start the thread. It avoids the null reference and
thread could be final, can't remember if I made the Thread reference
final or not.
Oddly enough though , Hudson failed shortly after, so I'll need to
investigate why, here's the Hudson web site:
http://hudson.zones.apache.org/hudson/view/River/job/River-trunk/
Cheers,
Peter.
Christopher Dolan wrote:
OK, sorry for the repeated messages. The list apparently doesn't allow
attachments...
--- TaskManager.java 2010-03-02 11:49:30.629703600 -0600
+++ TaskManager2.java 2010-06-08 11:11:54.309412900 -0500
@@ -190,10 +190,13 @@
logAdd(t); //DAS 6/06 add
boolean poke = true;
while (threads.size() < maxThreads && needThread()) {
- Thread th;
+ TaskThread th;
try {
th = new TaskThread();
- th.start();
+ Thread thread = new Thread(new TaskThread());
+ thread.setName("task");
+ thread.setDaemon(true);
+ thread.start();
} catch (Throwable tt) {
try {
logger.log(threads.isEmpty() ?
@@ -290,8 +293,8 @@
for (int j = threads.size(); --j >= 0; ) {
TaskThread thread = (TaskThread)threads.get(j);
if (thread.task == t) {
- if (thread != Thread.currentThread())
- thread.interrupt();
+ if (thread.myThread !=
Thread.currentThread())
+ thread.myThread.interrupt();
break;
}
}
@@ -327,15 +330,11 @@
return maxThreads;
}
- private class TaskThread extends Thread {
+ private class TaskThread implements Runnable {
/** The task being run, if any */
public Task task = null;
-
- public TaskThread() {
- super("task");
- setDaemon(true);
- }
+ public Thread myThread = null;
/**
* Find the next task that can be run, and mark it taken by
@@ -363,8 +362,9 @@
}
public void run() {
+ myThread = Thread.currentThread();
while (true) {
synchronized (TaskManager.this) {
if (terminated)
return;
if (task != null) {
@@ -376,11 +376,11 @@
}
}
task = null;
- interrupted(); // clear interrupt bit
+ myThread.interrupted(); // clear interrupt bit
}
if (!takeTask()) {
try {
TaskManager.this.wait(timeout);
} catch (InterruptedException e) {
}
if (terminated || !takeTask()) {
-----Original Message-----
From: Christopher Dolan [mailto:[email protected]]
Sent: Tuesday, June 08, 2010 11:13 AM
To: [email protected]
Subject: RE: com.sun.jini.thread lock contention
Oops, let me try that patch one more time...
Chris
-----Original Message-----
From: Christopher Dolan
Sent: Tuesday, June 08, 2010 11:11 AM
To: '[email protected]'
Subject: RE: com.sun.jini.thread lock contention
Attached is a minimalist initial patch. I'm not happy that I had to
store a reference to the thread in the Runnable... But otherwise I
wasn't sure how to interrupt it from removeTask() without a separate map
of TaskThread to Thread. There's all a null pointer race in this patch
if you try to remove a task before it's run() method executes.
Chris
