Re: Join thread with SwingWorker objects
On 27 nov, 20:01, Lew <l...@lewscanon.com> wrote:
... Try implementing
something like the following piece of code
private final Object lock = new Object();
private volatile int countCompleted;
pricate volatile int activeProcesses;
You're controlling these variables with different monitors - sometimes
'lock', sometimes the internal one due to being 'volatile'. I'm not
convinced that the relationship between these variables is reliably
readable.
the countCompleted and activeProcesses do not have to be synchronized
via the lock object they just need to be written immediately by the
JVM (so no caching). The countCompleted is only used to update the
progressbar so if the number would be off (like i.e. 1 to less) for a
few milliseconds then nobody is noticing it. On the activeProcesses I
agree the volatile can be removed (it is only read by the main
thread).
public void processCompleted(BatchExtractionItem completedItem) {
//System.out.println("Completed "+completedItem);
synchronized(lock) {
activeProcesses--;
countCompleted++;
lock.notifyAll(); // wake u=
p the main thread
}
}
enum State { WAITING, RUN_NEXT };
protected Integer doInBackground() throws Exception {
State state = State.WAITING; // forgot this line ...
List<BatchExtractionItem> queuedItems = new
ArrayList<BatchExtractionItem>();
queuedItems.addAll(items);
activeProcesses = 0;
countCompleted = 0;
boolean keepRunning = true;
BatchExtractionItem itemToRun = null;
while (keep_running) {
switch(state) {
The read of 'state' is not synchronized with the write.
case WAITING : {
synchro=
nized(lock) {
if (activeProcesses<MAX_ACTIVE_PROCESSES) {
if (queuedItems.isEmpty()) {
if (activeProcesses==0) {
keep_running = f=
alse;
break;
}
} else {
state = FIND_TO_RUN;
break;
}
}
try {
lock.wait(20000l); // w=
ait for 20 seconds max
(or a notify from processCompleted) and then check again
=
} catch(Exception ignore) {}
}
} break;
case RUN_NEXT : {
BatchEx=
tractionItem item = queuedItems.removeLast(queuedItems);
You don't synchronize the change to 'queuedItems'.
if (!it=
em.getStatus().equals(BatchExtractionItem.COMPLETED) && !
or the 'getStatus()' read.
item.getStatus().equals(BatchExtractionItem.PROCESSING)) {
=
item.setStatus(BatchExtractionItem.PROCESSING);
BatchTask task = new BatchTask(item);
task.execute();
activeProcesses++;
{
// spitt out a warning
System.err.println("warn: next item was already processing or has
// countCompleted++; // add this if it should be counted as a
= WAITING;
} break;
}
}
// no further checking needed ... all items have finise=
d processing
return 0;
}
@Override
public void actionPerformed(ActionEvent e) {
//issued by Timer event
int progress = countCompleted/items.size();
setProgress(progress);
form.getProgressBar().setValue(progress);
}
I'm having difficulty reasoning about the synchronization the way
you've written all this. I suspect there are subtle threading bugs
there.
--
Lew
The variable state is only written and read by one thread so there is
no need for synchronization. About the part of the getStatus I have to
partly agree with you: the getStatus() internally calls the method
FutureTask.isDone() which than calls sync.innerIsDone() where the sync
is of type Sync (see:
http://www.google.com/codesearch/p?hl=en&sa=N&cd=2&ct=rc#atE6BTe41-=
M/libcore/concurrent/src/main/java/java/util/concurrent/FutureTask.java&q=
=FutureTask)
It seems to me that it is overly synced (but at the time of writing I
forgot to check. I assumed hole did the right job there)