From: John B. Matthews on
In article
<cb502594-5385-4d4e-9a45-9646caa99a79(a)j19g2000yqk.googlegroups.com>,
Hole <h0leforfun(a)gmail.com> wrote:

> May you give me any suggestion for the following?
[...]
> class SchedulerTask extends SwingWorker<Integer, Void> ...
[...]
> class BatchTask extends SwingWorker<List<SampledValue>, Void> ...

I can't see where you define Void, "the type used for carrying out
intermediate results by this SwingWorker's publish and process methods"
and symbolized by the generic parameter V in the API [1].

You might look at the examples in the SwingWorker API [1] and this
similar database example [2]. In the latter, the generic type V is
Double, and individual instances are published from the background
thread. Note that the List<Double> received by process() is _not_ the
same List<Double> returned by doInBackground(). The former contains
intermediate results; the latter is the background thread's working
result set.

The method doInBackground() can create other threads, but you still have
to synchronize their processing, as others have suggested.

[1]<http://java.sun.com/javase/6/docs/api/javax/swing/SwingWorker.html>
[2]<http://sites.google.com/site/drjohnbmatthews/randomdata>

[...]
--
John B. Matthews
trashgod at gmail dot com
<http://sites.google.com/site/drjohnbmatthews>
From: Lew on
John B. Matthews wrote:
> I can't see where you define Void, "the type used for carrying out
> intermediate results by this SwingWorker's publish and process methods"
> and symbolized by the generic parameter V in the API [1].
>

<http://java.sun.com/javase/6/docs/api/java/lang/Void.html>

> You might look at the examples in the SwingWorker API [1] and this
> similar database example [2]. In the latter, the generic type V is
> Double, and individual instances are published from the background
> thread. Note that the List<Double> received by process() is _not_ the
> same List<Double> returned by doInBackground(). The former contains
> intermediate results; the latter is the background thread's working
> result set.
>
> The method doInBackground() can create other threads, but you still have
> to synchronize their processing, as others have suggested.
>
> [1]<http://java.sun.com/javase/6/docs/api/javax/swing/SwingWorker.html>
> [2]<http://sites.google.com/site/drjohnbmatthews/randomdata>
>


From: markspace on
John B. Matthews wrote:

>> class SchedulerTask extends SwingWorker<Integer, Void>

> I can't see where you define Void,


As Lew mentioned, it's part of the java.lang package.

A trick is to define SwingWorker<Void,Void> and then return "null" from
doInBackground():

class ExampleWorker
extends SwingWorker<Void, Void>
{
...
@Override
protected Void doInBackground() {
...
return null;
}

I notice the OP is declaring "Integer" but always returning 0. "Void"
might actually suit him better.

See my example up thread.
From: John B. Matthews on
In article <hepben$99l$1(a)news.eternal-september.org>,
markspace <nospam(a)nowhere.com> wrote:

> > John B. Matthews wrote:
> >
> > > class SchedulerTask extends SwingWorker<Integer, Void>
>
> > I can't see where you define Void,

> At nearly the same time, Lew wrote:

> <http://java.sun.com/javase/6/docs/api/java/lang/Void.html>

> As Lew mentioned, it's part of the java.lang package.

Ah, thank you both.

> A trick is to define SwingWorker<Void,Void> and then return "null" from
> doInBackground():
>
> class ExampleWorker
> extends SwingWorker<Void, Void>
> {
> ...
> @Override
> protected Void doInBackground() {
> ...
> return null;
> }
>
> I notice the OP is declaring "Integer" but always returning 0.

I wondered about this.

> "Void" might actually suit him better.
>
> See my example up thread.

Exemplary!

--
John B. Matthews
trashgod at gmail dot com
<http://sites.google.com/site/drjohnbmatthews>
From: Douwe on
On 27 nov, 20:01, Lew <l...(a)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 up 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 : {
> >                                 synchronized(lock) {
> >                                         if (activeProcesses<MAX_ACTIVE_PROCESSES) {
> >                                                 if (queuedItems.isEmpty()) {
> >                                                         if (activeProcesses==0) {
> >                                                                 keep_running = false;
> >                                                                 break;
> >                                                         }
> >                                                 } else {
> >                                                         state = FIND_TO_RUN;
> >                                                         break;
> >                                                 }
> >                                         }
> >                                         try {
> >                                                 lock.wait(20000l);              // wait for 20 seconds max
> > (or a notify from processCompleted) and then check again
> >                                         } catch(Exception ignore) {}
> >                                 }
> >                          } break;
>
> >                          case RUN_NEXT : {
> >                                 BatchExtractionItem item = queuedItems.removeLast(queuedItems);
>
> You don't synchronize the change to 'queuedItems'.
>
> >                                 if (!item.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++;
> >                                 } else {
> >                                         // spitt out a warning
> >                                         System.err.println("warn: next item was already processing or has
> > completed");
> >                                         // countCompleted++;   // add this if it should be counted as a
> > completed task
> >                                 }
> >                                 state = WAITING;
> >                         } break;
> >                 }
> >         }
> >         // no further checking needed ... all items have finised 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)