Is it good practice to submit Runnable to a thread pool in a loop?

Question

I have the following code snippet and I'm wondering if submitting instances of Runnable in a loop is good practice when running tasks on a thread pool.

I need to have access to the list outside the loop which is my reasoning. This is pseudo code, so my real code uses ConcurrentHashMap's, eliminating thread issues. If this is bad practice, does anybody have any better recommendations? I tried splitting this off into another class, but ran into issues with my outside list.

I had troubles knowing when to clear the list out of memory, I had no way of knowing when the threads all completed.

public void startJob() {
    int threads = Runtime.getRuntime().availableProcessors();
    ExecutorService exec = Executors.newFixedThreadPool(threads);

    final List<ImportTask> importTasks = session.createCriteria(ImportTask.class).list();
    final List<Object> objs = new ArrayList<>();

    int count = 0;

    for (ImportTask importTask : importTasks) {
        exec.submit(new Runnable() {
            @Override
            public void run() {
                count++;

                if(objs.contains(importTask) {
                    obj = objs.get(importTask.indexOf(importTask));
                } else {
                    Object obj = new Object();
                    objs.add(obj);
                    session.save(obj);
                }


                if(count % 50 = 1000) {
                    session.flush();
                    session.commit();
                }
            }
        }
    }
}

This question appears to be off-topic because it is about reviewing (presumably working) code. Please ask it on http://codereview.stackexchange.com — Bohemian, Nov 03 '13 at 23:55
@Bohemian This question is completely on topic, there is no need for me to post my exact objects. I'm just asking if looping the ExecutorService with the the runnable is good practice. I am using this loop in my current code, but fear issues. — Code Junkie, Nov 04 '13 at 00:08
It would obviously be better to write the loop inside the run() method, if it's possible. Using an ExecutorService as an *ersatz* 'for' loop is certainly not good practice. — user207421, Nov 04 '13 at 00:10
@EJP This is where I believe my issue is, I need to multi thread the importTask which is the reason for keeping it outside of the exec service. Putting it inside of the run method wouldn't enable me to multi thread the importtask, any other suggestions? — Code Junkie, Nov 04 '13 at 00:14
this guy seems to have a similar issue, http://stackoverflow.com/questions/12845881/java-splitting-work-to-multiple-threads should I not be using Runnable? I wasn't aware I could create new threads without it. — Code Junkie, Nov 04 '13 at 00:28
Then you aren't looping at all, you are submitting concurrent tasks. So what's the question? — user207421, Nov 04 '13 at 00:56
I just built the following example, http://www.javacodegeeks.com/2013/01/java-thread-pool-example-using-executors-and-threadpoolexecutor.html What I'm trying to do is separate my WorkerThread "runnable" into another class, but have it update a shared list outside of the ImportTask loop which would be shared by all threads. The problem was I didn't know how to prevent the list from just clearing as soon as the threads started which lead me to wonder if looping the exec was good practice. The example in the link seems to be using while(!executor.isTerminated), is this the correct way? — Code Junkie, Nov 04 '13 at 04:30

Boj · Accepted Answer · 2013-11-05T00:19:57.003

Submitting instances of Runnable to a thread pool is a perfectly valid way of achieving the concurrent execution of your tasks.

To know when each task is done, keep a list of the Future objects returned by ExecutionService.submit().

Future<?> future = exec.submit(new Runnable() {
...
futures.add(future);

In the end get the result of each future using the blocking Future.get().

As suggested by another answer, you can also use ExecutorService.invokeAll(). The difference is that that method only takes Callable and not Runnable, have to submit all tasks at one go, and the method waits until the last task terminates.

A further step that would make your code much neater is making your task (ImportTask) implement the Callable interface (and the call method) and return it's computation. If you need to pass values into the task, such as your objs list, you can do so via a constructor of ImportTask.

thanks for your response. What would be the difference in using submit over pasha701's answer of using invokeAll? — Code Junkie, Nov 04 '13 at 14:47

score 1 · Answer 2 · answered Nov 04 '13 at 11:01

1

For know when all threads are completed, you can use "ExecutorService.invokeAll" instead of "submit".

answered Nov 04 '13 at 11:01

pasha701

6,831
1
15
22

What's the difference between invokeAll and Wlll's suggestion of Future? – Code Junkie Nov 04 '13 at 14:35
If you make "Future> future = exec.submit(new Runnable().." and next statement will "future.get()", current thread will wait for Runnable end, only one thread will be run. With "invokeAll" many threads are run, and current thread will continue only when all thread are ended. – pasha701 Nov 06 '13 at 10:05

Is it good practice to submit Runnable to a thread pool in a loop?

2 Answers2