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

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 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

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();
}