CODEWITHSUNDEEP
javamultithreading
hudi
hudi

Reputation: 16555

How to wait for all threads to complete

I created some workflow how to wait for all thread which I created. This example works in 99 % of cases but sometimes method waitForAllDone is finished sooner then all thread are completed. I know it because after waitForAllDone I am closing stream which is using created thread so then occurs exception 

Caused by: java.io.IOException: Stream closed

my thread start with:

  @Override
  public void run() {
    try {
      process();
    } finally {
      Factory.close(this);
    }
  }

closing: 

  protected static void close(final Client client) {
    clientCount--;
  }

when I creating thread I call this:

  public RobWSClient getClient() {
    clientCount++;
    return new Client();
  }

and clientCount variable inside factory:

  private static volatile int clientCount = 0;

wait:

  public void waitForAllDone() {
    try {
      while (clientCount > 0) {
        Thread.sleep(10);
      }

    } catch (InterruptedException e) {
      LOG.error("Error", e);
    }
  }

Upvotes: 0

Views: 1626

Answers (3)

maasg
maasg

Reputation: 37435

The best way to wait for threads to terminate, is to use one of the high-level concurrency facilities. In this case, the easiest way would be to use an ExecutorService.

You would 'offer' a new task to the executor in this way:

...
ExecutorService executor = Executors.newFixedThreadPool(POOL_SIZE);
...

Client client = getClient(); //assuming Client implements runnable
executor.submit(client);
...

public void waitForAllDone() {
    executor.awaitTermination(30, TimeUnit.SECOND) ; wait termination of all threads for 30 secs
... 
}

In this way, you don't waste valuable CPU cycles in busy waits or sleep/awake cycles. See ExecutorService docs for details.

Upvotes: 1

John B
John B

Reputation: 32969

You need to protect the modification and reading of clientCount via synchronized. The main issue is that clientCount-- and clientCount++ are NOT an atomic operation and therefore two threads could execute clientCount-- / clientCount++ and end up with the wrong result.

Simply using volatile as you do above would ONLY work if ALL operations on the field were atomic. Since they are not, you need to use some locking mechanism. As Anton states, AtomicInteger is an excellent choice here. Note that it should be either final or volatile to ensure it is not thread-local.

That being said, the general rule post Java 1.5 is to use a ExecutorService instead of Threads. Using this in conjuction with Guava's Futures class could make waiting for all to complete to be as simple as:

Future<List<?>> future = Futures.successfulAsList(myFutureList);
future.get();
// all processes are complete

Futures.successfulAsList

Upvotes: 5

Anton
Anton

Reputation: 6061

I'm not sure that the rest of your your code has no issues, but you can't increment volatile variable like this - clientCount++; Use AtomicInteger instead

Upvotes: 3

Related Questions