CODEWITHSUNDEEP
javathread-safetysynchronized
Kevin Meredith
Kevin Meredith

Reputation: 41939

Java Threads "synchronized"

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?

Upvotes: 6

Views: 2053

Answers (6)

Samuel
Samuel

Reputation: 17171

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.

Upvotes: 6

Dustin Getz
Dustin Getz

Reputation: 21811

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);

Upvotes: 0

Pshemo
Pshemo

Reputation: 124295

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.

Upvotes: 1

Andre
Andre

Reputation: 162

cant you use 

   private volatile int foo;

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

Upvotes: 0

rgettman
rgettman

Reputation: 178363

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;
}

Upvotes: 4

radai
radai

Reputation: 24212

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

Upvotes: 6

Related Questions