Is this java class thread safe?

Question

This isn't homework for me, it's a task given to students from some university. I'm interested in the solution out of personal interest.

The task is to create a class (Calc) which holds an integer. The two methods add and mul should add to or multiply this integer.

Two threads are set-up simultaneously. One thread should call c.add(3) ten times, the other one should call c.mul(3) ten times (on the same Calc-object of course).

The Calc class should make sure that the operations are done alternatingly ( add, mul, add, mul, add, mul, ..).

I haven't worked with concurrency related problems a lot - even less with Java. I've come up with the following implementation for Calc:

class Calc{

    private int sum = 0;
    //Is volatile actually needed? Or is bool atomic by default? Or it's read operation, at least.
    private volatile bool b = true;

    public void add(int i){
        while(!b){}
        synchronized(this){
                sum += i;
            b = true;   
        }
    }

    public void mul(int i){
        while(b){}
        synchronized(this){
            sum *= i;
            b = false;  
        }
    }

}

I'd like to know if I'm on the right track here. And there's surely a more elegant way to the while(b) part. I'd like to hear your guys' thoughts.

PS: The methods' signature mustn't be changed. Apart from that I'm not restricted.

Answer 1

Hmmm. There are a number of problems with your solution. First, volatile isn't required for atomicity but for visibility. I won't go into this here, but you can read more about the Java memory model. (And yes, boolean is atomic, but it's irrelevant here). Besides, if you access variables only inside synchronized blocks then they don't have to be volatile.

Now, I assume that it's by accident, but your b variable is not accessed only inside synchronized blocks, and it happens to be volatile, so actually your solution would work, but it's neither idiomatic nor recommended, because you're waiting for b to change inside a busy loop. You're burning CPU cycles for nothing (this is what we call a spin-lock, and it may be useful sometimes).

An idiomatic solution would look like this:

class Code {
    private int sum = 0;
    private boolean nextAdd = true;

    public synchronized void add(int i) throws InterruptedException {
        while(!nextAdd )
            wait();
        sum += i;
        nextAdd = false;
        notify();
    }

    public synchronized void mul(int i) throws InterruptedException {
        while(nextAdd)
            wait();
        sum *= i;
        nextAdd = true;
        notify();
    }
}

Answer 2

What you did is a busy loop: you're running a loop which only stops when a variable changes. This is a bad technique because it makes the CPU very busy, instead of simple making the thread wait until the flag is changed.

I would use two semaphores: one for multiply, and one for add. add must acquire the addSemaphore before adding, and releases a permit to the multiplySemaphore when it's done, and vice-versa.

private Semaphore addSemaphore = new Semaphore(1);
private Semaphore multiplySemaphore = new Semaphore(0);

public void add(int i) {
    try {
        addSemaphore.acquire();
        sum += i;
        multiplySemaphore.release();
    }
    catch (InterrupedException e) {
        Thread.currentThread().interrupt();
    }
}

public void mul(int i) {
    try {
        multiplySemaphore.acquire();
        sum *= i;
        addSemaphore.release();
    }
    catch (InterrupedException e) {
        Thread.currentThread().interrupt();
    }
}

Answer 3

Try using the Lock interface:

class Calc {

    private int sum = 0;
    final Lock lock = new ReentrantLock();
    final Condition addition  = lock.newCondition(); 
    final Condition multiplication  = lock.newCondition(); 

    public void add(int i){

        lock.lock();
        try {
            if(sum != 0) {
                multiplication.await();
            }
            sum += i;
            addition.signal(); 

        } 
        finally {
           lock.unlock();
        }
    }

    public void mul(int i){
        lock.lock();
        try {
            addition.await();
            sum *= i;
            multiplication.signal(); 

        } finally {
           lock.unlock();
        }
    }
}

The lock works like your synchronized blocks. But the methods will wait at .await() if another thread holds the lock until .signal() is called.

Answer 4

Yes, volatile is needed, not because an assignment from a boolean to another is not atomic, but to prevent the caching of the variable such that its updated value is not visible to the other threads who are reading it. Also sum should be volatile if you care about the final result.

Having said this, it would probably be more elegant to use wait and notify to create this interleaving effect.

class Calc{

    private int sum = 0;
    private Object event1 = new Object();
    private Object event2 = new Object();

    public void initiate() {
        synchronized(event1){
           event1.notify();
        }
    }

    public void add(int i){
        synchronized(event1) {
           event1.wait();
        }
        sum += i;
        synchronized(event2){
           event2.notify();
        }
    }

    public void mul(int i){
        synchronized(event2) {
           event2.wait();
        }
        sum *= i;
        synchronized(event1){
           event1.notify();
        }
    }
}

Then after you start both threads, call initiate to release the first thread.

Answer 5

As others have said, the volatile in your solution is required. Also, your solution spin-waits, which can waste quite a lot of CPU cycles. That said, I can't see any problems as far as correctness in concerned.

I personally would implement this with a pair of semaphores:

private final Semaphore semAdd = new Semaphore(1);
private final Semaphore semMul = new Semaphore(0);
private int sum = 0;

public void add(int i) throws InterruptedException {
    semAdd.acquire();
    sum += i;
    semMul.release();
}

public void mul(int i) throws InterruptedException {
    semMul.acquire();
    sum *= i;
    semAdd.release();
}

Answer 6

volatile is needed otherwise the optimizer might optimize the loop to if(b)while(true){}

but you can do this with wait and notify

public void add(int i){

    synchronized(this){
        while(!b){try{wait();}catch(InterruptedException e){}}//swallowing is not recommended log or reset the flag
            sum += i;
        b = true;   
        notify();
    }
}

public void mul(int i){
    synchronized(this){
        while(b){try{wait();}catch(InterruptedException e){}}
        sum *= i;
        b = false;  
        notify();
    }
}

however in this case (b checked inside the sync block) volatile is not needed

Answer 7

The program is fully thread safe:

1. The boolean flag is set to volatile, so the JVM knows not to cache values and to keep write-access to one thread at a time.

2. The two critical sections locks on the current object, which means only one thread will have access at a time. Note that if a thread is inside the synchronized block, no thread can be in any other critical sections.

The above will apply to every instance of the class. For example if two instances are created, threads will be able to enter multiple critical sections at a time, but will be limited to one thread per instances, per critical section. Does that make sense?