Reputation: 41939
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
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
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
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
Reputation: 162
cant you use
private volatile int foo;
or atomic http://docs.oracle.com/javase/tutorial/essential/concurrency/atomicvars.html
Upvotes: 0
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
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