CODEWITHSUNDEEP
javamultithreading
Joan Font
Joan Font

Reputation: 81

Issue with Java threads and synchronized

I have a "Counter" class that increments a counter until a limit. I create N instances of this class, and every class increases the counter until MAX_COUNT/N. I have implemented synchronized method, but the counter never reaches the MAX_COUNT value (i'm going to post the code, it will help to understand the problem)

Main.java

package monitores;

// MAIN PROGRAM, IT CREATES THE THREADS, RUNS THEM AND JOINS, AT THE END
// DISPLAYS THE COUNTER VALUE

public class Main {


    public static void main(String[] args) throws InterruptedException {

        Hilo hilos[] = new Hilo[Hilo.NUM_THREADS];
        for(int i = 0; i < Hilo.NUM_THREADS; i++){
            hilos[i] = new Hilo();
            hilos[i].start();
        }

        for(int i = 0; i < Hilo.NUM_THREADS; i++){
            hilos[i].join();
        }

        System.out.println("Counter value at the end: "+Hilo.count());
    }

}

Hilo.java

package monitores;


public class Hilo extends Thread {

    public static final int MAX_COUNT = 40000;
    public static final int NUM_THREADS = 4;

    private static volatile int count = 0;

    public static int count() {
        return count;
    }


    @Override
    public void run() {
        int max = MAX_COUNT / NUM_THREADS;
        for (int i = 0; i < max; i++) {
            synchronized(this){
                count++;
            }
        }
    }
}

If i run Main.java, te result is the following:

Counter value at the end: 33870

This a random number, but always is close to the MAX_COUNT.

Thank you in advance.

Upvotes: 1

Views: 103

Answers (3)

Gray
Gray

Reputation: 116938

but the counter never reaches the MAX_COUNT value (i'm going to post the code, it will help to understand the problem)

The problem is as @rgettman mentions that you are synchronizing on this which is per-thread instead of a shared object such as the class.

Another solution would be to switch to using an AtomicInteger which handles the synchronization and memory sharing for you:

private static AtomicInteger count = new AtomicInteger(0);
...
public static int count() {
    return count.get();
}
...
for (int i = 0; i < max; i++) {
    // no need for synchronization object here
    count.incrementAndGet();
}

Upvotes: 0

Montaldo
Montaldo

Reputation: 863

This smells like a race condition. If you have random values every time that means different threads are writing to the same variable at the same moment.

If I have two threads you need to synchronize the shared variable or create a synchronized method that they all need to use to update count.

Upvotes: 0

rgettman
rgettman

Reputation: 178363

Your threads are only synchronizing on themselves. All they do is lock themselves and increment, so the code is not threadsafe.

They need to synchronize on a common object. In this case, the .class object is reasonable. Each thread has easy access to that one single object on which to lock:

synchronized(Hilo.class){
    count++;
}

An alternative is not to synchronize, but to replace the datatype for count with an AtomicInteger. Increment with getAndIncrement().

Upvotes: 3

Related Questions