Why variable is not visible to other thread in synchronization?

Question

Let's assume I've two threads t1 and t2 which are trying to access incX()

Here is my following code:

    class Test implements Runnable {
    private int x  = 0;

    public void incX() {
    synchronized(this) {
    x = ++x; 
    }
    System.out.println("x is: "+x+"     "+Thread.currentThread().getName());
   }

    public void run() {
    incX();
}

 public static void main(String[] args)   {

   Thread t1 = new Thread(new Test());
   t1.start();
   Thread t2 = new Thread(new Test());
   t2.start();

}

Here's my output:

    x is: 1     Thread-1
    x is: 1     Thread-0

As in incX() method I've synchronized x = ++x, so the changes made to thread t1 should be visible to thread t2, right? So my output should be:

    x is: 1     Thread-1
    x is: 2     Thread-0

I know ++x is not an atomic operation but it is synchronized, so thread t2 can't acquire the lock. So the threads should not interleave and changes made to x should be visible to thread t2, right? Am I misunderstanding? So my question is why I am not getting the output:

     x is: 2     Thread-0

Answer 1

You're using two separate instances of your Test class, so naturally the x in each of them only gets incremented once. It's effectively the same as this:

Test test1 = new Test();
Test test2 = new Test();
test1.incX();
test2.incX();

Since each instance of Test has its own x, you'll see 1 twice with that code too.

To test synchronized access to the same instance, you need to use a single instance instead. For example:

class Test {
    private int x  = 0;

    public void incX() {
        synchronized(this) {
            x = ++x;                         // See "Side Note" below
        }
        System.out.println("x is: "+x+"     "+Thread.currentThread().getName());
    }
}
public class Main {
    public static void main(String[] args) {
        Test test = new Test();              // One instance
        Thread t1 = new Thread(() -> {
            test.incX();                     // Used by this thread
        });
        Thread t2 = new Thread(() -> {
            test.incX();                     // And also by this one
        });
        t1.start();
        t2.start();
        try {
            t1.join();
            t2.join();
        }
        catch (Exception e) {
        }
        System.out.println("Done");
    }
}

Which will output

x is: 1     Thread-0
x is: 2     Thread-1
Done

or similar (naturally it varies depending on which thread gets scheduled when).

Sometimes, it'll even look like this:

x is: 2     Thread-0
x is: 2     Thread-1
Done

That's because the access to x in the System.out.println statement is outside the synchronized block, so sometimes (not always) x will get incremented after the end of the synchronized block and before the println:

synchronized(this) {
    x = ++x;
}
// ***The other thread can jump in here and increment x
System.out.println("x is: "+x+"     "+Thread.currentThread().getName());

In more detail:

1. t1 enters the synchronized block
2. t2 tries to enter the synchronized block but has to wait because t1 has the lock
3. t1 increments x, making it 1
4. t1 exits the synchronized block
5. t2 jumps in and increments x, making it 2
6. t2 exits the synchronized block
7. t1 outputs the current value of x (2)
8. t2 outputs the current value of x (2)

Note that Step 2 and Step 3 could be in any order, and Steps 6-8 could also be in any order.

To reliably report x as it was within the synchronized block after being incremented, we'd want to either:

1. Move the println into the synchronized block

   public void incX() {
       synchronized(this) {
           x = ++x;                         // See "Side Note" below
           System.out.println("x is: "+x+"     "+Thread.currentThread().getName());
       }
   }
   

   or

2. Save the result of the increment in a local variable

   public void incX() {
       int y;                               // Local variable
       synchronized(this) {
           y = ++x;                         // No longer and odd thing to do
       }
       System.out.println("x is: "+y+"     "+Thread.currentThread().getName());
   }
   

Unless you have a really, really good reason to hold a synchronization lock during output, go with #2.

---

Side note: As I mentioned in a comment, ++x already writes its value back to x, that's what the increment operators do. So the x = part of

x = ++x;

...is unnecessary. Either use the increment operator:

++x;

...or don't:

x += 1;