views:

178

answers:

2
List sychronizedList = Collections.synchronizedList(new ArrayList());  //is this ok?

try {

    ExecutorService executor = Executors.newFixedThreadPool(10);
    for (final Object item : fromCollection) {
         executor.submit(new ARunnableClass(sychronizedList));
    }
    executor.shutdown();  //do i need this
                          // line as i want to wait for all threads
                          // finished executed to return result in
                          // java.util.List

    executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);

    //reset
    executor=null;

} catch (InterruptedException ex) {
    Logger.getLogger(ThisClass.class.getName()).log(Level.SEVERE,
                 null, ex);
}

1.may i know is it correct to use shared-List Collections.synchronizedList for all my threads?
2. is it a need to put the line executor.shutdown() as I want to wait for all my threads to finish processing no matter how long . A result will will add into list sychronizedList

+2  A: 

Yes the shutdown() is required. shutdown() tells the threadpool that no more jobs can be submitted (it'll throw an exception if you try) and it means that the awaitTermination() will run until all the jobs finish (at what point the thread pool will shut down). The shutdown() is basically what awaitTermination() is waiting for.

It's a way of making the thing block until all work is done. There are other ways to do this (eg a CountdownLatch) but this way is pretty good.

As for whether you need to synchronize your List or not, well that depends on what you're doing with it.

Perhaps I should give you a framework for how to do this in a generic sense.

public interface Function<T> {
  T execute(T input);
}

public static <T> List<T> map(List<T> input, final Function<T> func)
    throws InterruptedException, ExecutionException {
  List<Future<T>> futures = new ArrayList<Future<T>>(input.size());
  ExecutorService executor = Executors.newFixedThreadPool(10);
  for (final T item : input) {
    futures.add(executor.submit(new Callable<T>() {
      @Override
      public T call() {
        return func.execute(item);
      }
    }));
  }
  executor.shutdown();
  executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
  List<T> ret = new ArrayList<T>(futures.size());
  for (Future<T> future : futures) {
    ret.add(future.get());
  }
}

Not 100% sure this will work as is as I've just written it off the top of my head but the principles are sound. You can then do:

List<Integer> input = new ArrayList<Integer>();
for (int i=0; i<100; i++) {
  input.add(i);
}
List<Integer> result = map(input, new Function<Integer>() {
  @Override
  public Integer execute(Integer in) {
    return in * in;
  }
});

It's about the best you can do in Java without closures or delegates.

How it works:

  • It creates an intermediate list of Futures;
  • Futures are the result of an operation. If the operation hasn't completed yet, calling Future.get() will block until it does return, otherwise it returns the result immediately;
  • Each item of the list is passed in to the function passed in and mapped to a new value. This way the mapped operation doesn't need to be aware of any threading;
  • The thread pool is shutdown. No more submissions are allowed;
  • The awaitTermination() will block until the last thread finishes executing.

The above can be adapted to be synchronous too, which is useful because your functions are thread-agnostic.

Edit: as noted, technically you can get away without the awaitTermination(), instead simply doing a Future.get() since that will block but that method isn't as clean and may hide thread pool errors. Basically, shutdown/awaiTermination is just better. Alos if you get an exception on Future.get() you'll need to make sure you shutdown the thread pool in a finally block anyway.

cletus
just one more little thing, do u think is good prastice to put executor=null; after finished processing? normally i will put var as null . what do u think..
cometta
the null is not needed at all in this case since the executor goes out of scope at the end of each for loop iteration and it is available for garbage collection. Generally you don't need to set things to null unless they are class/instance variables. The one exception that springs to mind is an object that is no longer needed and the rest of the method is going to potentially take a long time (such as something used at the start of main but not used anymore before the real work is done).
TofuBeer
Good work!!! +1
Adeel Ansari
Actually you DON'T need the shutdown() and awaitTermination(). Since get() will block you can just use it on all your jobs (whether or not new jobs are added is irrelevant, since you have the Futures of the jobs you're interested in).Furthermore it depends on the problem at hand whether the Function<T> defined in this answer is needed, maybe just creating the Callable<T> implementations when you call the function is just as easy as Function...
Fried Hoeben
@Fried: yes, technically that is correct but IMHO it's less clear. It also ignored possible thread pool errors. Also, you can use awaitTermination() to specify a timeout more easily than you can by blocking on individual futures. It's just cleaner, basically.
cletus
+1  A: 

Minor point:

List sychronizedList = Collections.synchronizedList(new ArrayList());  //is this ok?

should probably be:

List<Object> sychronizedList = Collections.synchronizedList(new ArrayList<Object>());

And then the Object should probably be changed to something else.

Also the line:

executor=null;

is not needed since the executor is declared wth the for loop it becomes eligible for gc as soon as the current for loop iteration ends.

TofuBeer
how about for normal method like below public void dosomething(){ List abc= new Array(); //do something abc=null; } need to do this since no loop?
cometta
if the method is small (in terms of speed) no need at all. If the list is used for a while, then not used anymore, and there is part of the method that takes significant time (either directly or through method calls) then maybe. I am not sure if hotspot is smart enough to know when a varialbe is no longer is use, but I bet it can. I personally try to make everything final which prevents me from setting things to null... I never have gc issues...
TofuBeer
do u mean all variables in methods you put as final ? like ---> void test(final abc){ final Long testagain=new Long(3); } ? put all variables final?
cometta
probably 95% of all my variables, class, instance, parameters, and locals are marked as final. It is unusual for variables to change once you have set them up.
TofuBeer