Multithread security with synchronized in Java

Question

I'm implementing a program that calculates a Julia set. It will use multiple threads, depending on how many processors are available. Each thread calculates a line, but only when that line is not being calculated by another thread. This part WORKS pretty well.

But sometimes when I test it with bigger images (more lines to calculate, for example instead of getHeight() = 1200, I set it to 3000, there are some lines which are skipped). I want to make it more secure, so that no line will be calculated twice, and no lines will be skipped. Here is the code of the run() method:

 public void run() {
     while (counter < getHeight()-1) {  
         synchronized(this) {
             if (counter >= getHeight() -1) { //so that the last line will not be calculated >2 times.
                 return;
             }
             counter++;
             image.setRGB(0, counter, getWidth(), 1, renderLine(counter), 0, 0);
         }
     }
  }

I want it to work like that: if the current line is being calculated, the thread goes to the next line.. without that it get confused, so that lines get skipped..

I'm trying this actually:

 public void run() {
     while (counter < getHeight()-1 && !working) {  
         synchronized(this) {
             working = true;
             if (counter >= getHeight() -1) { //so that the last line will not be calculated >2 times.
                 return;
             }
             counter++;
             image.setRGB(0, counter, getWidth(), 1, renderLine(counter), 0, 0);
             working = false;
         }
     }
  }

but I don't know if it will prevent access to another thread, while a thread is already working, and it will change the value of "counter", meaning that lines can be skipped!

Do I need a boolean variable to notify that a thread is actually working on a line? Any advice?

Answer 1

You would really need to have a shared object for all your threads. This object will tell the other threads which line to work on.

I can't tell definitely what you have now, but it appears each synchronized is on a different instance in which you lose all mutual exclusion. Remember that synchronizing only works for multiple threads when the synchronization occurs on shared objects, otherwise each thread will sync on thread local objects in which nothing is achieved.

Here is an example

public class SharedLineCounter{
   private final int maxNumberOfLines;
   private int currentLineNumber =0;
   public SharedLineCounter(int maxNumberOfLines){
      this.maxNumberOfLines = maxNumberOfLines;
   } 
   public synchronized int getNextLine(){
      if(++currentLineNumber > maxNumberOfLines)
         return -1; //end case
      return currentLineNumber ;
   }
}

public class WorkerThread extends Thread{
    private final SharedLineCounter counter;
    public WorkerThread(SharedLineCounter counter){
       this.counter = counter;
    }
    public void run(){
      int next = -1;
      while((next = counter.getNextLine()) >= 0){
        image.setRGB(0, next , getWidth(), 1, renderLine(next ), 0, 0);
      }
    }
  }
}

Here each thread will share this thread safe line counter so each thread should always get a unique and sequential line number.

Edit to answer your question:

You can make the Thread anonymous by sharing a global instance

public static void main(String args[]){
    final SharedCounter counter = new SharedCounter();

    Thread worker1 = new Thread(new Runnable(){
       public void run(){
            counter.getNextLine(); //etc
       }
    });
    Thread worker2 = new Thread(new Runnable(){
       public void run(){
            counter.getNextLine(); //etc
       }
    });

}

One caveat to address based on your comment. You should pass the new Runnable in and create an anonymous runnable, its bad practice to subclass and override the run method of the Thread

Answer 2

You're almost certainly doing too much of your own thread management. Use an ExecutorService to distribute the work between multiple threads without duplication.

 ExecutorService service = Executors.newFixedThreadPool(
   Runtime.getRuntime().availableProcessors());
 for (int row = minRow; row <= maxRow; row++) {
   service.submit(new FillThisRowRunnable(row));
 }