Java Threads "synchronized"

Question

This article talks about Java's "synchronized" keyword.

  ...
  private int foo;
  public synchronized int getFoo() { return foo; } 
  public synchronized void setFoo(int f) { foo = f; }

If a caller wants to increment the foo property, the following code to do so is not thread-safe:

  ...
  setFoo(getFoo() + 1);

If two threads attempt to increment foo at the same time, the result might be that the value of foo gets increased by one or by two, depending on timing.

Now, my question:

Why doesn't "synchronized" on setFoo() prevent the above bolded line?

Calcine

Answer 1

This is an example of a check-then-act race condition.

A scenario might happen like the following:

Thread-1 getFoo() returns 0
Thread-2 getFoo() returns 0
Thread-2 setFoo(1)
Thread-1 setFoo(1)

This would mean that two threads have attempted to increment foo but it has the effect of only being incremented once.

As other answers have identified, synchronizing the incrementation with a synchronized block locking on the same object as getFoo() and setFoo() will prevent this race condition because threads won't be able to interleave like above.

Narration

Answer 2

because youre guaranteed no one else is getting foo alongside you, and that no one else is setting foo back besides you, but youre NOT guaranteed no one managed to get in and out (or just in) between you calling get() and you calling set()

you can think of that code as completely equivalent to this:

int temp = getFoo(); //safe method
temp = temp+1; //not protected here - im not holding any locks ...
setFoo(temp); //safe method

Dillman

Answer 3

The synchronized keyword on both methods doesn't make it thread safe, because one thread could call getFoo, then another thread could call getFoo, and each of them get the same result. Then each of them add one and call setFoo, and the end result is that foo gets incremented only once, instead of twice. As your article points out, this is a race condition.

To make it thread safe, both the read and the write must be together in the same synchronized block, without separate get and set methods.

public synchronized void addFoo(int addend)
{
   foo += addend;
}

Anfractuosity

Then each of them add one and call setFoo, and the end result is that foo gets incremented only once, instead of twice Why? – Calcine

Because each Thread updates foo to the same value. For example, each of them gets the value 2, each of them adds 1 to get 3, then each of them sets the value to 3. – Anfractuosity

If I understand right, then shouldn't your statement be Then each of them add one and call setFoo, and the end result is that foo gets incremented **twice**? – Calcine

It's a matter of semantics now. Sure, both threads increment foo, so it is incremented "twice". But each time, each thread increments it to 3, instead of one thread incrementing it to 3 and then the other thread incrementing it to 4. – Anfractuosity

Answer 4

Main trap in your code is that it may seem that getFoo will be called "inside" setFoo. Kind of

setFoo(){
   //getFoo();
   //...
}

which is incorrect because in reality getFoo is called before calling setFoo. Here is example that shows it:

public static int foo(int i) {
    System.out.print("FOO!");
    return i;
}

public static int bar(int i) {
    System.out.print("BAR!");
    return i;
}

public static void main(String[] args) throws Exception {
    System.out.println(foo(bar(1)));
}

Output:

BAR!FOO!1

As you can see bar was invoked before foo. So in your case it is possible that two (or more) threads will invoke getFoo which will return current value before they will call setFoo. In this case they will both have same value, lets say 0 and when they will call setFoo they will both set it to 1.

Nasal

Answer 5

cant you use

   private volatile int foo;

or atomic http://docs.oracle.com/javase/tutorial/essential/concurrency/atomicvars.html

Combat

volatile wont solve it, but atomic incrementation is good advice – Nasal

Answer 6

does this code help?

class C {
  private int foo;
  public int getFoo() { return foo; } 
  public void setFoo(int f) { foo = f; }
}

C myC = new C();
synchronized(myC) {
  int foo = myC.getFoo();
  myC.setFoo(foo + 1);
}
println(myC.foo);

Drivein

So the point is that myC.setFoo(1) could get called before println(myC.getFoo())? – Calcine