CODEWITHSUNDEEP
javamultithreadingconcurrency
csvan
csvan

Reputation: 9474

Could I join threads in a better way?

Suppose I have multiple Runnable instances in a program, all dispatched by an Executor instance. Further, suppose I at some point need to wait for a subset of these runnables to finish before moving on.

One way I could do this is the following:

public abstract class Joinable implements Runnable {

    private final Semaphore finishedLock = new Semaphore(1);

    @Override
    public final void run() {
        try {
            finishedLock.acquireUninterruptibly();
            doWork();
        } finally {
            finishedLock.release();
        }
    }

    public abstract void doWork();

    public void join() {
        finishedLock.acquireUninterruptibly();
    }
}

Implementing classes can then simply override doWork(), rather than run(), in order to define what should be done during execution.

The joining process will then simply look like this:

void doStuff() {

    Executor executor = Executors.newCachedThreadPool();

    List<Joinable> joinables = new LinkedList<Joinable>();
    // Fill joinables with implementors of Joinable...

    List<Runnable> others = new LinkedList<Runnable>();
    // Fill others with implementors of Runnable...

    for(Joinable joinable : joinables) 
        executor.execute(joinable);

    for(Runnable runnable : others) 
        executor.execute(runnable);

    for(Joinable joinable : joinables) 
        joinable.join();

    // Continue, no matter what the threads in others are up to.
}

Is this a good way to solve this problem (is it even safe?), or is there a better one?

Upvotes: 1

Views: 476

Answers (2)

Gray
Gray

Reputation: 116908

Is this a good way to solve this problem (is it even safe?)

This is not quite right. You can't join on a Runnable that you are executing by the ExecutorService. If you want to use a list then do something like this:

List<Future<?>> futures = new ArrayList<Future<?>>();
for(Joinable joinable : joinables) {
   // this submit returns a `Future`.
   futures.add(executor.submit(joinable));
}
// submit others to the executor _without_ adding to the futures list

for (Future<?> future : futures) {
    // this can throw ExecutionException which wraps exception thrown by task
    future.get();
}

or is there a better one?

If you were waiting for all tasks to complete then you could use the ExecutorService.awaitTermination(long timeout, TimeUnit unit) method. For example:

executor.awaitTerminate(Long.MAX_VALUE, TimeUnit.MILLISECONDS);

But I don't see any better way to do this if you are waiting for a subset of tasks.

Upvotes: 1

Laplie Anderson
Laplie Anderson

Reputation: 6429

Your current solution is not thread safe. There are no guarantees that the executor will call run on your Joinable before you call join. Thus, in certain cases, your main thread will acquire the lock before your Joinable does.

On possible solution would be instead to use a CountDownLatch if you know the total number of joinables N, you create a CountDownLatch(N) and pass it to each instance. When each joinable is finished, have it call countDown(). Your main thread calls await() on the latch. await() doesn't return until the latch count is 0.

Upvotes: 2

Related Questions