java - cachedThreadPool not working as I expected -

i'm playing around threads , don't understand why isn't working thought.

i trying calculate sum using threads , expecting thread pool wait tasks finish time print out result (due shutdown() call , isterminated() check).

what missing here?

import java.util.concurrent.executorservice; import java.util.concurrent.executors;  public class test5 {  private integer sum= new integer(0);  public static void main(string[] args) {      executorservice pool = executors.newcachedthreadpool();      test5 obj = new test5();      for(int i=0; i<1000; i++){             pool.execute(obj.new adding());     }      pool.shutdown();      while(!pool.isterminated()) {         //could empty loop...         system.out.println(" done? : " + pool.isterminated());     }      system.out.println(" done? : " + pool.isterminated());     system.out.println("sum " + obj.sum);                 }  class adding implements runnable {      public void run() {          synchronized(this) {              int tmp = sum;             tmp+=1;             sum=new integer(tmp);                    }                            }                } } 

while results, output such this:

is done? : true sum 983 

you have number of issues.

1. your code is not threadsafe
2. busy waiting anti-pattern avoided.

what mean 1.?

lets suppose have 2 threads, & b.

1. a reads sum tmp 1
2. b reads sum tmp 1
3. a increments sum 2
4. a writes sum 2
5. b increments sum 2
6. b writes sum 2

so end 2 after 2 increments. no quite right.

now may "but have used synchronized, should not happen". well, haven't.

when create adding instances new each one. have 1000 separate adding instances.

when synchronized(this) synchronizing on current instance, not across adding. synchronized block nothing.

now, simple solution use synchronized(adding.class).

the synchronized(adding.class) make code block synchronized correctly across adding instances.

the good solution use atmoicinteger rather integer increments atomically , designed sort of task.

now onto 2.

you have while(thing){} loop, runs thread crazy testing thousands of times millisecond until thing true. a huge waste of cpu cycles. executorservice has special, blocking, method waits until has shutdown, awaittermination.

here example:

static final atomicinteger sum = new atomicinteger(0);  public static void main(string[] args) throws interruptedexception {      executorservice pool = executors.newcachedthreadpool();      (int = 0; < 1000; i++) {         pool.execute(new adding());     }      pool.shutdown();     pool.awaittermination(1, timeunit.days);      system.out.println(" done? : " + pool.isterminated());     system.out.println("sum " + sum); }  static class adding implements runnable {      public void run() {         sum.addandget(1);     } } 

i suggest not using cachedthreadpool in circumstance have 1000 runnables being submitted , generate far more threads have cpus. suggest using newfixedthreadpool sane number of threads.

i'm not going go use of int literals , integer , why new integer() not needed.