Java Thread Shared Object Synchronization Issue

Question

I'm having issues with Synchronized not behaving the way i expect, i tried using volatile keyword also:

Shared Object:

public class ThreadValue {
private String caller;
private String value;
public ThreadValue( String caller, String value ) {
    this.value = value;
    this.caller = caller;
}

public synchronized String getValue() {
    return this.caller + "     "  + this.value;
}
public synchronized void setValue( String caller, String value ) {
    this.caller = caller;
    this.value = value;
}
}

Thread 1:

class CongoThread implements Runnable {
    private ThreadValue v;
    public CongoThread(ThreadValue v) {
    this.v = v;

    }
    public void run() {
    for (int i = 0; i  10; i++) {
    v.setValue( "congo", "cool" );
    v.getValue();
    }
    }
}

Thread 2:

class LibyaThread implements Runnable {
    private ThreadValue v;
    public LibyaThread(ThreadValue v) {
    this.v = v;

    }
    public void run() {
    for (int i = 0; i  10; i++) {
       v.setValue( "libya", "awesome" );
       System.out.println("In Libya Thread " + v.getValue() );

    }
    }
}

Calling Class:

class TwoThreadsTest {
    public static void main (String args[]) {

    ThreadValue v = new ThreadValue("", "");
        Thread congo = new Thread( new CongoThread( v ) );
        Thread libya = new Thread( new LibyaThread( v ) );

    libya.start();
        congo.start();

    }
}

Occasionally i get "In Libya Thread congo cool" which should never happen. I expect only: "In Libya Thread libya awesome" "In Congo Thread congo cool"

I dont expect them to be mixed.

Answer 1

The problem is, it's possible that the v.getValue() for the Libya thread is being called right after the Congo thread has called v.setValue(), thus resulting in your mix up.

The solution is to have a thread block BOTH the get and set values, otherwise you're going to continue to have that problem. You'll have to call the getter inside of the setter or use a wait/notify to have the other thread wait until the one thread has both set and gotten the value.

Answer 2

Why would they not be mixed? Although each individual call is synchronized, there's nothing to stop one thread from calling v.setValue, then the other thread calling setValue, then the first thread calling getValue(). I believe that's what's happening. You could avoid this by using:

public void run() {
    for (int i = 0; i  10; i++) {
       synchronized (v) {
           v.setValue( "libya", "awesome" );
           System.out.println("In Libya Thread " + v.getValue() );
       }
    }
}

That way, on each iteration it makes sure that it calls setValue and getValue without another thread calling setValue in the meantime.

It's not an ideal design, admittedly - but I'm guessing this demonstration is more to understand synchronization than anything else :)

Answer 3

What happens is the follwing:

1. Thread 1 sets the value
2. Thread 2 sets the value
3. Thread 1 reads the value set by thread 2.

In order to fix this you need to have 1 lock object that guards the get/set functions call for both threads. The best way to do this, is to make an extra synchronized method that does both the set and the get. However sometimes that isn't desirable. In that case give both threads a lock object. Which is just a plain object. Which they then use in a synchronized block.

Implementation of each thread would like like the following, note that they need to have precisely the same object!

Object lockObject = new Object();
Thread t1 = new CongroThread(v, lockObject);
Thread t2 = new LibyaThread(v, lockObject);

...

class CongoThread implements Runnable {
    private ThreadValue v;
    private Object lockObject;

    public CongoThread(ThreadValue v, Object lockObject) {
    this.v = v;
    this.lockObject = lockObject,
    }
    public void run() {
        for (int i = 0; i < 10; i++) {
            synchronized(lockObject)
            {
                v.setValue( "libya", "awesome" );
                System.out.println("In Libya Thread " + v.getValue() );
            }
        }
    }
}