Java Synchronized and Threads

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 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", "lame" );
            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

You're only synchronizing access to getValue and setValue separately not the two-liner

v.setValue( "congo", ..);
v.getValue();

Then of course the two threads may interlace between one's setValue and getValue

Answer 2

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( "congo", "lame" );
                v.getValue();
            }
        }
    }
}

Answer 3

Did you synchronze the System.out.print calls ? Without synchronize, they are thread safe, but may not emit in the correct order.

synchronzied(System.out) {
    System.out.print(....);
    System.out.flush();
}