How to handle stale data in java concurrency?

Question

I am following the book 'Java Concurrency In Practice' to understand Java Concurrency in detail. Then i came across a term called 'Stale Data'.

Book Says: Insufficiently Synchronized programs can cause surprising results-->Stale data.

There is an example given in the book, which protects the mutator methods using 'synchronized' Keyword & uses '@GuardedBy' annotation on its field. I thought of testing it.

import net.jcip.annotations.*;
public class ThreadTest4 extends Thread{
    
    private MyWork4 myWork= new MyWork4();
    
    public static void main(String[] args){
        ThreadTest4 thread1 = new ThreadTest4();
        ThreadTest4 thread2 = new ThreadTest4();
        ThreadTest4 thread3 = new ThreadTest4();
        ThreadTest4 thread4 = new ThreadTest4();
        
        thread1.start();
        thread2.start();
        thread3.start();
        thread4.start();
        
    }
    
    public void run(){
            myWork.setA();
            System.out.println(myWork.getA());
    }
}

class MyWork4{
    
    @GuardedBy("this")  private static int a;
    
    public synchronized int getA(){
        return a;   
    }
    
    public synchronized void setA(){
        a++;
    }
    
}

But it still surprises me with its results! What can be the reason?

Answer 1

You have two problems.

First, your myWork object, on which you are locking, is private to each thread, so the threads only lock each other out when calling setA() which attempts to modify the static int, which requires the lock on the FIRST OBJECT in order to allow the value to change. Thus, aside from Thread1, none of the getA() calls are dependent on waiting on the lock.

The second problem, which stems from this, is that your set and get calls are overlapping. @GuardedBy prevents the item from being modified by an item that doesn't have the lock, and the synchronized methods can only be invoked by a caller that owns the lock. All of the threads register their setA() calls, but have to wait for the locks to modify the value. Thread1 starts with the lock, and modifies the value, then releases the lock, then requests it again with its getA() call.

Then, Thread2's setA() call is executed when Thread1 releases the lock. Thread2 releases the lock after incrementing the value, and then registers its request for the lock with its own getA() call.

Thread1 gets the lock back now to perform its waiting getA() call, and prints out 2 because by this time, Thread1 and Thread2 have already modified the value.

Thread3 gets the lock next and performs its waiting setA() call, and increments the value again and releases the lock, and registers its getA() call.

Thread 2 then gets the lock back for its waiting getA() call and prints out 3 and releases the lock.

Thread 3's waiting getA() call happens next, and prints out 3 again because no other setter calls have happened yet.

Finally, Thread4, which was started last, gets to run, and registers its setA() call incrementing the value, and then prints out the newly incremented getA() call because it's no longer waiting for the lock.

Your run method isn't synchronized, and your individual threads have nothing to order themselves on except which requests the lock first, which is dependent on enough different factors as to be essentially random.

Here's a modification which lets your order be more predictable:

public class ThreadTest4 extends Thread {
    static Object lock = new Object[0];
    private MyWork4 myWork = new MyWork4();

    public static void main(String[] args) {
        ThreadTest4 thread1 = new ThreadTest4();
        ThreadTest4 thread2 = new ThreadTest4();
        ThreadTest4 thread3 = new ThreadTest4();
        ThreadTest4 thread4 = new ThreadTest4();

        thread1.start();
        thread2.start();
        thread3.start();
        thread4.start();
    }

    public void run() {
        synchronized(lock){
            myWork.setA();
            System.out.println(myWork.getA());   
        }
    }
}

class MyWork4 {

    @GuardedBy("lock")
    private static int a;

    public synchronized int getA() {
        return a;
    }

    public synchronized void setA() {
        a++;
    }

}

This works because the lock is external and explicitly shared among the threads. All of the threads use the same lock, and so they execute their setA() calls and getA() calls in order before the next thread is given the locks, which lets them play more nicely.