3

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?

enter image description here

Oliv
  • 10,221
  • 3
  • 55
  • 76
Renjith
  • 1,122
  • 1
  • 19
  • 45
  • @user2864740 did you mean calling setA() & getA() should be inside a synchronized block.? i guess that is what you meant by atomic – Renjith Jan 03 '17 at 03:59

1 Answers1

3

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.

Steve K
  • 4,863
  • 2
  • 32
  • 41
  • synchronized(lock) inside run() worked. But why do we need an external lock. why it is not working when we use the internal lock of 'myWork' object!?. I believe it should stop multiple threads from accessing the 'myWork' methods. – Renjith Jan 03 '17 at 04:25
  • Renju, your myWork instance is private to each thread. They can't see each others' instances, so they can't use them to order correctly. If it were static, the different threads could share it and use it for orderly locking, but since they're all private they essentially don't block each other the way they should. – Steve K Jan 03 '17 at 04:31
  • Ok.. Make sense. So if i use a static instance of MyWork4, it can be used as a lock. let me try that. correct me if i am wrong. – Renjith Jan 03 '17 at 04:37
  • Cool!!... that worked. I forget the fact that 'myWork' is not shared as it is private to each thread. Hey, I guess this would work even if I remove the 'synchronized' keyword from each method – Renjith Jan 03 '17 at 04:43
  • I noticed that your suggestion will work even if I don't keep 'synchronized' & '@GuardedBy' on MyWOrk4 class. – Renjith Jan 03 '17 at 05:02