Is running a ExecutorService inside a SwingWorker a good practice?

Question

Consider following code:

        SwingWorker<Void, Void> sworker = new SwingWorker<Void, Void>() {

        @Override
        protected Void doInBackground() throws Exception {
            ExecutorService executor = Executors.newFixedThreadPool(5);
            try {
                for (int j = 0; j < 5; j++) {
                    Callable<Object> worker = new MyCallableImpl();
                    Future<Object> future = executor.submit(worker); 
                    array[j] = future.get();
                }
            } catch (InterruptedException e) {
                // some code here
            } catch (ExecutionException e) {
                // some code here
            }
                // some code here
            executor.shutdown();
            return null;
        }

    };
    sworker.execute();

As I said in the title: is this a good practice to invoke ExecutorService inside doInBackground() method of SwingWorker? It works for me (JDK1.7), GUI is not blocked and multiple threads from Executor pool are running in background, but still I have some doubts...

score 2 · Answer 1 · answered Mar 28 '12 at 14:52

2

The above code doesn't make much sense to me.

If the objective here is to ensure that the GUI remains responsive while a long-running task is being executed, then there's no need to use the ExecutorService since the SwingWorker already provides that mechanism.

answered Mar 28 '12 at 14:52

mre

43,520
33
120
170

Yes, I know it, but I want to run multiple threads (Callables) inside SwingWorker. How can I do it without unnecessary wrapping Executor inside SwingWorker? – DoktorNo Mar 28 '12 at 14:55
I would just get rid of the `SwingWorker` altogether then. If any of these tasks modify Swing component(s), wrap the call using `SwingUtilities.invokeLater` – mre Mar 28 '12 at 14:58
Yes, they are modifying Swing components (code is not shown, to avoid confusion). I'll try your solution in the meantime. – DoktorNo Mar 28 '12 at 15:00
I wrapped piece of code that was in doInBackground in SwingUtilities.invokeLater(), inside a anonymous Runnable, and GUI got blocked. This is not what I intended... – DoktorNo Mar 28 '12 at 15:10

John Vint · Answer 2 · 2012-03-28T15:54:23.677

To further mre's response. It doesn't make sense because your execution is actually single-threaded. The doInBackground will submit to the executor and wait for that single task to complete then submit another.

You should submit the same way, but store the returned Futures in a List of some sort then get on each one of them after all tasks have been submitted.

I don't as much mind the doInBackground to submit these jobs asynchronously as mre does. If you are trying to submit a number of tasks and have only N submitted at any given time you definitely shouldn't do this through SwingWorker.doInBackground. Using an ExectorService + SwingUtilities.invokeLater I think is the better way.

And just to clarify any confusion, invokeLater should only be used here when the task within the ExecutorService is complete and all it needs to do is update the UI component.

Edit: Example to address your comment

protected Void doInBackground() throws Exception {
    ExecutorService executor = Executors.newFixedThreadPool(5);
    List<Future> futures = ...;
    try {
        for (int j = 0; j < 5; j++) {
            Callable<Object> worker = new MyCallableImpl();
            futures.add(executor.submit(new Callable<Object>(){
                 public Object call(){
                    //expensive time consuming operation
                    final String result = ...;//result from consuming operation
                    SwingUtilities.invokeLater(new Runnable(){
                        public void run(){
                             jLabel.setText(result);
                        }
                    });
                    return new Object();
                 }
            ));
        }
        for(Future<Object> f :futures)f.get();
        executor.shutdown();
    return null;
}

Notice how the invokeLater is done to do a simple update? That should not cause your EDT to freeze.

I'll try this. BTW, what is the difference between SwingUtilities.invokeLater() and EventQueue.invokeLater()? — DoktorNo, Mar 28 '12 at 15:27
In the standard Java distribution, nothing. `SwingUtilities.invokeLater` just delegates to `EventQueue.invokeLater`. Just couples all necessary functionality into SwingUtilities. — John Vint, Mar 28 '12 at 15:30
I wrapped all calls for changing the GUI in invokeLater, inside a anonymour Runnable, and GUI is still locked. What I did wrong? BTW: the whole code wrom my snippet is run in method invoked by event listener (after pressing a button). — DoktorNo, Mar 28 '12 at 15:49
Can you post your code. The updated the GUI via invokeLater should usually be a single assignment. All the expensive work should be within the ExecutorService, I'll edit with an example — John Vint, Mar 28 '12 at 15:50
@DoktorNo Feel free to take a look at my edit to see what I am talking about. — John Vint, Mar 28 '12 at 15:54
Thank you, I'll ceck this later, because its evening in my locatiob :) I need to rest, and then I'll analyze your solution. Thank you all for help. — DoktorNo, Mar 28 '12 at 15:59

score 2 · Answer 3 · edited May 23 '17 at 12:30

2

can executing SwingWorkers instance from Executor
have to accepting that Executor doesn't care about SwingWorkers lifecycle and vice versa
have to implement PropertyChangeListener for SwingWorker
exmple here

edited May 23 '17 at 12:30

Community

1
1

answered Mar 28 '12 at 15:17

mKorbel

109,525
20
134
319

Is running a ExecutorService inside a SwingWorker a good practice?

3 Answers3

Linked