views:

726

answers:

4

Hi, i am using a ThreadPoolExecutor with a thread pool size of one to sequentially execute swing workers. I got a special case where an event arrives that creates a swing worker that does some client-server communication and after that updates the ui (in the done() method).

This works fine when the user fires (clicks on an item) some events but not if there occur many of them. But this happens so i need to cancel all currently running and scheduled workers. The problem is that the queue that is backing the ThreadPoolExecutor isnt aware of the SwingWorker cancellation process (at least it seems like that). So scheduled worker get cancelled but already running workers get not.

So i added a concurrent queue of type <T extends SwingWorker> that holds a reference of all workers as long as they are not cancelled and when a new event arrives it calls .cancel(true) on all SwingWorkers in the queue and submits the new SwingWorker to the ThreadPoolExecutor.

Summary: SwingWorkers are created and executed in a ThreadPoolExecutor with a single Thread. Only the worker that was submitted last should be running.

Are there any alternatives to solve this problem, or is it "ok" to do it like this?

Just curious...

A: 

Why do you need a ThreadPoolExecutor to do this kind of job?

How many sources of different SwingWorkers you have? Because if the source is just one you should use a different approach.

For example you can define a class that handles one kind of working thread and it's linked to a single kind of item on which the user can fire actions and care inside that class the fact that a single instance of the thread should be running (for example using a singleton instance that is cleared upon finishing the task)

Jack
I need a threadpool executor with a single thread to ensure that only one task is running at a time. but there cann occure many of them, so i have to queue them.
MrWhite
A: 

Instead of using a SwingWorker could you not use a ThreadPoolExecutor to perform the client-server communication and then call SwingUtilities.invokeLater to update the UI with the result? This to me seems a bit cleaner and would ensure that events and UI updates are still processed in order.

As you submit tasks to your executor you could keep a reference to their Future instances so that you can cancel the task if needed.

Mark
The problem is that the future that i can get from the executor does not provide the cancellation possibilities like the swing worker because it just wraps the runnable interface.
MrWhite
But they both provide the same cancel method. SwingWorker implements the Future interface.
Mark
Like i said. It seems like the Executor isnt aware of it. And just executes it as a Runnable
MrWhite
A: 

Let me see if I understand the problem correctly. You have a FIFO queue of tasks, only the oldest of which is running. Each task needs to update the UI when it's done. But if a certain user event comes in, all tasks need to be cancelled -- that is, running tasks need to be cancelled, and not-yet-running tasks need to be removed from the queue. Is that right?

Assuming it is, I wouldn't use SwingWorker since you only need one worker thread, not one per task. FutureTask should be enough (assuming you override done() to make the necessary call to SwingUtilities.invokeLater() and do the UI update).

If you cancel a FutureTask, then even if its run() method gets called, it won't do anything. So you can submit FutureTasks safely to a ExecutorService knowing that cancellation will work even if the executor tries to run them.

I suspect that a good-enough solution would just be to keep a list of all FutureTasks that might need to be cancelled, and cancel them all when the user event comes in. The ExecutorService will still try to run them but it'll basically be a no-op. You need to make sure completed tasks are removed from the list, and you need to make sure the list is updated and used in a thread-safe way (probably from the same thread that puts tasks on the ExecutorService), but that shouldn't be too hard.

I bashed the code below out in just an hour and I wouldn't bet on it being correct, but you get the idea. :)

/** Untested code! Use at own risk. */
public class SwingTaskExecutor {

    // ////////////////////////////////////////////////////////////
    // Fields

    private final ExecutorService execSvc = Executors.newFixedThreadPool(1);

    private final Lock listLock = new ReentrantLock();
    private final List<ManagedSwingTask<?>> activeTasks = 
            new ArrayList<ManagedSwingTask<?>>();

    // ////////////////////////////////////////////////////////////
    // Public methods

    public <T> Future<T> submit(SwingTask<T> task) {
     ManagedSwingTask<T> managedTask = new ManagedSwingTask<T>(task);
     addToActiveTasks(managedTask);
     execSvc.submit(managedTask);
     return managedTask;
    }

    public void cancelAllTasks() {
     listLock.lock();
     try {
      for (ManagedSwingTask<?> t: activeTasks) {
       t.cancel(true);
      }
      activeTasks.clear();
     } finally {
      listLock.unlock();
     }
    }

    // ////////////////////////////////////////////////////////////
    // Private methods

    private <T> void addToActiveTasks(ManagedSwingTask<T> managedTask) {
     listLock.lock();
     try {
      activeTasks.add(managedTask);
     } finally {
      listLock.unlock();
     }
    }

    // ////////////////////////////////////////////////////////////
    // Helper classes

    private class ManagedSwingTask<T> extends FutureTask<T> {

     private final SwingTask<T> task;

     ManagedSwingTask(SwingTask<T> task) {
      super(task);
      this.task = task;
     }

     @Override
     public void cancel(boolean mayInterruptIfRunning) {
      try {
       task.cancel();
      } finally {
       super.cancel(mayInterruptIfRunning);
      }
     }

     @Override
     protected void done() {
      removeFromActiveTasks();
      updateUIIfDone();
     }

     private void removeFromActiveTasks() {
      listLock.lock();
      try {
       activeTasks.remove(this);
      } finally {
       listLock.unlock();
      }
     }

     private void updateUIIfDone() {
      if (isDone()) {
       SwingUtilities.invokeLater(new Runnable() {
        @Override
        public void run() {
         task.updateUI();
        }
       });
      }
     }
    }

    public static interface SwingTask<T> extends Callable<T> {

     /** Called from the EDT if task completes successfully */
     void updateUI();

     /** Hook in case there's task-specific cancellation to be done*/
     void cancel();
    }
}

Something like that, anyway.

If you want to be doubly sure, you could then shut down and replace the ExecutorService, but that's probably not necessary.

David Moles
+1  A: 

One way to create a Single thread ThreadPoolExecutor that only executes last incoming Runnable is to subclass a suitable queue class and override all adding methods to clear the queue before adding the new runnable. Then set that queue as the ThreadPoolExecutor's work queue.

Bloodboiler
Nice. That's much cleaner than my idea of keeping a separate list of tasks needing cancellation.
David Moles
That is pretty much like my own implementation.
MrWhite