CODEWITHSUNDEEP
javaconcurrencysynchronization
janko
janko

Reputation: 4203

Do I need to synchronize on the result of an invokeAll call?

I am enhancing an existing algorithm that consists of multiple independent steps to use concurrent tasks. Each of the tasks will create multiple objects to hold its results. In the end, I would like to have a list of all the results to return from the controlling method. At the moment, my code looks something like that

private final ExecutorService pool = ...;

// A single task to be performed concurrently with other tasks.
private class WorkHorse implements Callable<Void> {
    private final Collection<X> collect;

    public WorkHorse(Collection<X> collect, ...) {
        this.collect = collect;
    }

    public Void call() {
        for (...) {
            // do work

            synchronized (this.collect) {
                this.collect.add(result);
            }
        }
        return null;
    }
}

// Uses multiple concurrent tasks to compute its result list.
public Collection<X> getResults() {
    // this list is supposed to hold the results
    final Collection<X> collect = new LinkedList<X>();

    final List<WorkHorse> tasks = Arrays.asList(  
        new WorkHorse(collect, ...), new WorkHorse(collect, ...), ...);
    this.pool.invokeAll(tasks);

    // ## A ##
    synchronized (collect) {
        return collect;
    }
}

Do I actually need the synchronized at "## A ##" to enforce a happens-before relationship with the modifying operations in the worker tasks? Or can I rely on all write operations to have happened after invokeAll returns and be visible to the controlling thread? And is there any reason, why I should not return the results collection from within its own synchronized block?

Upvotes: 4

Views: 3246

Answers (2)

David Moles
David Moles

Reputation: 51153

You don't need the second synchronized if you have the first one in there. As Zed notes, invokeAll() will block until all tasks have completed. Meanwhile, the synchronization around add() will ensure that the changes to the collection are visible to all threads, including the original calling thread.

As for whether you need the first one (which you didn't ask) -- I tried removing both synchronized blocks and couldn't actually get it to fail, but having it in there is probably the safer bet. According to the javadoc for LinkedList:

If multiple threads access a LinkedList concurrently, and at least one of the threads modifies the list structurally, it must be synchronized externally.

The other "2nd-generation" Collection implementations have similar warnings.

Note by the way that there's nothing magic about synchronizing on the collection itself. You could declare a separate mutex (any old Object would do) in the outer class, or synchronize on the outer class instance, and that would work just as well, so long as all WorkHorses synchronize on the same thing.

Upvotes: 0

Zed
Zed

Reputation: 57668

No, you don't need that. The documentation of invokeAll states that all jobs should be done when it returns. So there should be no further access to collect when you reach the return statement.

Upvotes: 3

Related Questions