3

I am trying to run multiple threads. I am clearly getting race condition and able to resolve it as follows:

final Data data = new Data();
for (int i = 0; i < numberOfThreads; i++) {
    final Thread thread = new Thread(new Runnable() {
        @Override
        public void run() {
            //using this sync block to stop the race condition 
            synchronized (data){
                final int value = data.getValue();
                data.setValue(value + 1);
            }
        }
    });
    thread.start();
}

But I do not want to sync on this block and instead want to handle it at the Data class. So I removed the above sync block and instead synced the get and set methods over at the Data class as follows but this still causes race conditions. Why the issue even though I have synced them?

public class Data {

    private int value;

    public synchronized int getValue(){
        return this.value;
    }

    public synchronized void setValue(int num){
        this.value = num;
    }
}
3
  • Because without the extra synch block, multiple threads are able to call getValue() simultaneously, inc it by one, then write the same value back, even though they should have incremented by the number of times getValue() was called.
    – markspace
    Apr 30, 2016 at 23:45
  • How exactly race condition manifests itself? Also, if intention is to have thread safe increment, why don't you use AtomicInteger ? Apr 30, 2016 at 23:45
  • This is a variation on double check locking (en.wikipedia.org/wiki/Double-checked_locking) - as the answers indicate, you're getting and then setting separately.
    – stdunbar
    May 1, 2016 at 0:00

4 Answers 4

4

Because you didn't. You synced either of them, so two threads cannot execute the methods at the same time, but one thread can execute getValue() then finishes getValue() and before it enters setValue() another thread gets its turn and calls getValue() which is perfectly legal and your race condition.

Btw. just in case Data would be your whole class, AtomicInteger would be the same, but done properly. There you e. g. have an incrementAndGet() method that does the read and write operation in one synchronisation block which is the essential point in your case.

2

Adding synchronized to the individual methods is similar to doing something like this

final Data data = new Data();
for (int i = 0; i < numberOfThreads; i++) {
    final Thread thread = new Thread(new Runnable() {
        @Override
        public void run() {
            synchronized (data){
                final int value = data.getValue();
            }
            synchronized (data){
                data.setValue(value + 1);
            }
        }
    });
    thread.start();
}

Where a thread could very clearly get stuck between the get and the set. To resolve this, you either need to add a new synchronized method to the Data class that accomplishes the value + 1 task, or wrap both lines in a synchronized block, as you have done in your code.

1

First of all, in your Data class value should be volatile.

Regarding the issue you mention; You can't change the code like you did. Since in the way you changed it to the following can happen:

1) value in Data is 0

2) Thread 0 reads value (reads 0)

3) Thread 1 reads value (reads 0)

4) Thread 1 increments value and writes the new value to Data.value (writes 1)

5) Thread 0 increments value and writes the new value to Data.value (writes 1)

The issue here is that in step 5) a 1 was written since Thread 0 doesn't know that Thread 1 already incremented value since Thread 0 read value.

5
  • The use of volatile is irrelevant if multiple threads only interact with this field inside of synchronized blocks. May 1, 2016 at 2:21
  • @KookieMonster volatile has 2 functions in Java; 1) To make sure that a partial value isn't read/written 2) To force the value to not be stored in a thread's local cache/memory In this case you obviously don't have to worry about case 1 (technically, you never have to worry about this with ints) However, unless you use volatile the different threads might not see the most recent value if the int is updated by another thread, due to in Java each thread having its own cache
    – Tmr
    May 1, 2016 at 3:27
  • In general, this is correct. But volatile isn't required to 'update' the values when using synchronized, as the values are already 'updated' at the end of a synchronized block (that's not to say that volatile and synchronized make the other redundant, you just don't need volatile in this particular situation). May 1, 2016 at 4:26
  • @KookieMonster I'm sorry but I don't follow you, can you please explain? If you update a non-volatile primitive from within a synchronized block in one thread, then why would you when reading the value from another thread be guaranteed to latest value? Does the Java specification guarantee that before/after a synchronized block the "resynchronization" of memory between threads/thread caches?
    – Tmr
    May 1, 2016 at 10:51
  • Before a monitor is released, writes are flushed from thread-local memory to shared memory. In your example, and in the original question, Thread #2 will be reading the value from within synchronized block (otherwise may try to read the field before Thread #1 has even executed the write instructions), thus will see any changes which Thread #1 made while holding the monitor. May 1, 2016 at 12:05
0

Your adding a synchronized on the method level. So one thread could be calling getValue() and one could be calling setValue(). You could remove the synchronised altogether and change the type of the private "value" member to AutomicInteger. Then utilise thread-safe method within that class.

1
  • It's not possible to synchronize on a method. The only thing your code can ever synchronize on is an object. A synchronized instance method synchronizes on the this object, and a synchronized class method synchronizes on the class object. May 1, 2016 at 2:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.