In java, can the use of synchronized methods be unreliable?

Question

Going straight to the point, i have made a code to test concurrency in java, using synchronized methods:

Code:

public class ThreadTraining {

    public static class Value {

        private static int value;

        public static synchronized void Add() {
            value++;
        }

        public static synchronized void Sub() {
            value--;
        }

        public static synchronized int Get() {
            return value;
        }
    }

    public static class AddT implements Runnable {

        public static String name;

        public AddT(String n) {
            name = n;
        }

        @Override
        public void run() {
            while (Value.Get() < 100) {
                int prev = Value.Get();
                Value.Add();
                System.out.println("[" + name + "] - changed value from " + prev + " to " + Value.Get());
            }
        }
    }

    public static class SubT implements Runnable {

        public static String name;

        public SubT(String n) {
            name = n;
        }

        @Override
        public void run() {
            while (Value.Get() > (-100)) {
                int prev = Value.Get();
                Value.Sub();
                System.out.println("[" + name + "] - changed value from " + prev + " to " + Value.Get());
            }
        }
    }

    public static void main(String[] args) {
        Thread threads[] = new Thread[3];
        for (int i = 0; i < 4; i++) {
            if (i % 2 == 0) {
                threads[i] = new Thread(new AddT("Adder - Thread #" + i));
            } else {
                threads[i] = new Thread(new SubT("Subtractor - Thread #" + i));
            }
            threads[i].start();
        }
    }
}

This code, tough, has an unreliable execution despite the fact "it is not possible for two invocations of synchronized methods on the same object to interleave." (Source: Oracle's concurrency tutorial - Synchronized methods), giving outputs that are unreliable, such as the following: (note, any non-patternful changes in the output are denoted between the "..." lines, not just unreliable behavior)

-----[Thread #0 - Adder] has been created!
=====[Thread #0 - Adder] has been started!
-----[Thread #1 - Subtractor] has been created!
[Thread #0 - Adder] - changed value from 0 to 1
[Thread #0 - Adder] - changed value from 1 to 2
[Thread #0 - Adder] - changed value from 2 to 3
...\*goes on, adding as expected, for some lines*\
[Thread #0 - Adder] - changed value from 83 to 84
[Thread #0 - Adder] - changed value from 84 to 85
-----[Thread #2 - Adder] has been created!
=====[Thread #1 - Subtractor] has been started!
[Thread #0 - Adder] - changed value from 85 to 86
[Thread #1 - Subtractor] - changed value from 86 to 85
[Thread #1 - Subtractor] - changed value from 86 to 85
[Thread #1 - Subtractor] - changed value from 85 to 84
...\*goes on, subtracting as expected, for some lines*\
[Thread #1 - Subtractor] - changed value from -98 to -99
[Thread #1 - Subtractor] - changed value from -99 to -100 \*This thread ends here, as it reaches the state where (value>(-100))==false*\
=====[Thread #2 - Adder] has been started!
[Thread #2 - Adder] - changed value from -100 to -99
[Thread #2 - Adder] - changed value from -99 to -98
[Thread #2 - Adder] - changed value from -98 to -97
...\*goes on as expected...*\
[Thread #2 - Adder] - changed value from -67 to -66
[Thread #2 - Adder] - changed value from -66 to -65
-----[Thread #3 - Subtractor] has been created!
=====[Thread #3 - Subtractor] has been started!
[Thread #3 - Subtractor] - changed value from -65 to -66
[Thread #3 - Subtractor] - changed value from -66 to -67
...\*goes on as expected...*\
[Thread #3 - Subtractor] - changed value from -71 to -72
[Thread #3 - Subtractor] - changed value from -72 to -73 \*NOTE: From -73 it goes to -74, without a Subtractor-action in between! WTF???*\
[Thread #2 - Adder] - changed value from -74 to -73
[Thread #2 - Adder] - changed value from -73 to -72
...\*goes on as expected...*\
[Thread #2 - Adder] - changed value from 98 to 99
[Thread #2 - Adder] - changed value from 99 to 100 \*This adder ends here, adder thread #0 probably ends after next line...but not before doing something crazy!*\
[Thread #0 - Adder] - changed value from 85 to 86 \*What the hell are these values doing here? Oh wait, next lines is...*\
[Thread #3 - Subtractor] - changed value from -73 to -47\*...Holy mother of god!*\
[Thread #3 - Subtractor] - changed value from 100 to 99
[Thread #3 - Subtractor] - changed value from 99 to 98
...
[Thread #3 - Subtractor] - changed value from -98 to -99
[Thread #3 - Subtractor] - changed value from -99 to -100 \*The logical nightmare is finally over.*\

Is the use of synchronized methods unreliable? Or is the implementation wrong? (If so, what is wrong then? and how to fix it?)

Answer 1

Here is the code which runs perfectly. Its complementing to the answers given by @Affe and @Aix here.

public class ThreadTraining {

    public static class Value {

        private static  int value;

        public static synchronized void Add() {
            value++;

        }

        public static synchronized void Sub() {
            value--;
        }

        public static synchronized int Get() {
            return value;
        }
    }

    public static class AddT implements Runnable {

        public static String name;

        public AddT(String n) {
            name = n;
        }

        @Override
        public void run() {

            while (Value.Get() < 100) {
                 synchronized(Value.class){
                int prev = Value.Get();
                Value.Add();
                System.out.println("[" + name + "] - changed value from " + prev + " to " + Value.Get());
            }
            }
        }
    }

    public static class SubT implements Runnable {

        public static String name;

        public SubT(String n) {
            name = n;
        }

        @Override
        public void run() {

            while (Value.Get() > (-100)) {
            synchronized(Value.class){
                int prev = Value.Get();
                Value.Sub();
                System.out.println("[" + name + "] - changed value from " + prev + " to " + Value.Get());
            }
            }
        }
    }

    public static void main(String[] args) {
        Thread threads[] = new Thread[3];
        for (int i = 0; i < 4; i++) {
            if (i % 2 == 0) {
                threads[i] = new Thread(new AddT("Adder - Thread #" + i));
            } else {
                threads[i] = new Thread(new SubT("Subtractor - Thread #" + i));
            }
            threads[i].start();
        }
    }
}

By not putting the synchronized block as shown in the modified code, you are letting threads grab the variable value without the operation being completed atomically. Hence the resulting inconsistency.

Answer 2

What are you seeing is not "unreliable" nor is it a implementation problem. You are dealing with a large number of race conditions. For example:

while (Value.Get() < 100) {
     // other threads could have called `Add()` or `Subtract()` here
     int prev = Value.Get();
     // other threads could have called `Add()` or `Subtract()` here
     Value.Add();
     // other threads could have called `Add()` or `Subtract()` here
     System.out.println("[" + name + "] - changed value from " + prev + " to " + Value.Get());
}

You call Get() 3 times in this loop and there is no guarantee that other threads have changed the value between each call to Get(). If you change your code to be like the following, it would produce more consistent output:

while (true) { {
     // only call get once
     int prev = Value.Get();
     if (prev >= 100) {
        break;
     }
     // return the new value from add
     int newValue = Value.Add();
     System.out.println("[" + name + "] - changed value from " + prev + " to " + newValue);
}

// here's the new version of add
public static synchronized int Add() {
    value++;
    return value;
}

Notice that there is still a race condition between the Get() and the Add() method call since it is a test and then a set.

Answer 3

Your implementation is a bit off. 'Get', 'Add' and 'Sub' are each locked, but there is a gap between your 'Get' and your addition or subtraction. The thread that just did 'Get' can take a break during that gap, and someone else changes the value. If you want multiple method calls to all happen as a single operation, you need to synchronize on something "larger" than the individual method.

synchronized (Value.class) {
  int prev = Value.Get();
  Value.Add();
  System.out.println("[" + name + "] - changed value from " + prev + " to " + Value.Get());
}

Note this still has a problem where <100 might not still be true when you enter, so you should recheck it. (and of course locking on an Class object isn't something you'd generally want to do in 'real' code :) )

Answer 4

To understand what's wrong with your code, consider the following:

int prev = Value.Get();
Value.Sub();
System.out.println(... + Value.Get());

Each of the three synchronized operations Get(), Sub(), Get() is guaranteed by to work correctly and atomically. However, there is no guarantee that another thread would not come in between those operations and modify Value.

Whenever you need to ensure the atomicity of a sequence of operations, you should either provide a single synchronized method that performs them all at once, or use an external synchronized block like so:

synchronized (Value.class) {
    int prev = Value.Get();
    Value.Sub();
    System.out.println(... + Value.Get());
}