Java Thread execution on same data

Question

first of all here is the code, you can just copy an paste

import java.util.ArrayList;

public class RepetionCounter implements Runnable{
private int x;
private int y;
private int[][] matrix;
private int xCounter;
private int yCounter;
private ArrayList<Thread> threadArray;
private int rowIndex;
private boolean[] countCompleted; 

public RepetionCounter(int x, int y, int [][]matrix)
{
    this.x = x;
    this.y = y;
    this.matrix = matrix;
    this.threadArray = new ArrayList<Thread>(matrix.length);
    this.rowIndex = 0;
    for(int i = 0; i < matrix.length; i++){
        threadArray.add(new Thread(this));
    }
    countCompleted = new boolean[matrix.length];
}

public void start(){
    for (int i = 0; i < threadArray.size(); i++){
        threadArray.get(i).start();
        this.rowIndex++;
    }
}

public void count(int rowIndex)
{
    for(int i = 0; i < matrix[rowIndex].length; i++){
        if (matrix[rowIndex][i] == x){
            this.xCounter++;
        } else if (matrix[rowIndex][i] == y){
            this.yCounter++;
        }
    }
}

@Override
public void run() {
    count(this.rowIndex);
    countCompleted[this.rowIndex] = true;
}

public int getxCounter() {
    return xCounter;
}

public void setxCounter(int xCounter) {
    this.xCounter = xCounter;
}

public int getyCounter() {
    return yCounter;
}

public void setyCounter(int yCounter) {
    this.yCounter = yCounter;
}

public boolean[] getCountCompleted() {
    return countCompleted;
}

public void setCountCompleted(boolean[] countCompleted) {
    this.countCompleted = countCompleted;
}

public static void main(String args[]){
    int[][] matrix = {{0,2,1}, {2,3,4}, {3,2,0}};
    RepetionCounter rc = new RepetionCounter(0, 2, matrix);
    rc.start();
    boolean ready = false;
    while(!ready){
        for(int i = 0; i < matrix.length; i++){
            if (rc.getCountCompleted()[i]){
                ready = true;
            } else {
                ready = false;
            }
        }
    }
    if (rc.getxCounter() > rc.getyCounter()){
        System.out.println("Thre are more x than y");
    } else {System.out.println("There are:"+rc.getxCounter()+" x and:"+rc.getyCounter()+" y");

    }
}

}

What I want this code to do: I give to the object a matrix and tow numbers, and I want to know how much times these two numbers occurs in the matrix. I create as many thread as the number of rows of the matrix (that' why there is that ArrayList), so in this object I have k threads (supposing k is the number of rows), each of them count the occurrences of the two numbers. The problem is: if I run it for the first time everything work, but if I try to execute it another time I get and IndexOutOfBoundException, or a bad count of the occurrences, the odd thing is that if I get the error, and modify the code, after that it will works again just for once. Can you explain to me why is this happening?

Answer 1

You are using the same instance of RepetitionCounter for each thread:

for(int i = 0; i < matrix.length; i++){
    threadArray.add(new Thread(this));
}

so they will all share the same rowIndex. The code is pretty confusing as it is, so I suggest you encapsulate the logic for the threads in a separate Runnable class with individual row ids:

class ThreadTask implements Runnable {
    private int rowId;
    private int[][] matrix;

    public ThreadTask(int[][] matrix, int rowId) {
        this.matrix = matrix; // only a reference is passed here so no worries
        this.rowId = rowId;
    }

    public void run() {
       // search on my row
    }
}

then:

for(int i = 0; i < matrix.length; i++) {
     threadArray.add(new Thread(new ThreadTask(matrix, i)));
}

Answer 2

You need to give each thread its own Runnable. Having them all share the same Runnable is going to cause disastrous race conditions. Separate out the logic each thread needs to do into a Runnable. Then move the part of the code that starts up the threads to a place outside the Runnable.

BTW look into Executors in the java.util.concurrent package, you don't have to use raw threads for this stuff. Also using Executors may give you a better idea about separating what goes into the Task from other stuff.