0

In a concurrent program, is this safe:


private volatile int i;

public int getI() {
    return i;
}

public synchronized incrementI() {
    i++;
}

?

From what I know about synchronize, the changes are guaranteed to be available on i only for threads that acquires the lock monitor on the same object. So I think, the code above is not safe. Please just confirm this if true, otherwise, please explain. Thanks.

artaxerxe
  • 6,281
  • 21
  • 68
  • 106

3 Answers3

1

This code is thread safe. Changes inside a synchronized method are passed on to all threads. And as you have made it volatile, all threads will not cache it, so will get latest copy.

Also this thread will be helpful Java volatile modifier and synchronized blocks

Community
  • 1
  • 1
Lokesh
  • 7,810
  • 6
  • 48
  • 78
  • 1
    Unfortunately, this answer is not correct. Neither "changes inside a synchronized method are passed on to all threads", nor "if you made it volatile, all threads will not cache it". This is a correct answer: http://stackoverflow.com/a/29446437/2613885 – Aleksey Shipilev Apr 04 '15 at 12:32
  • @AlekseyShipilev If you dont make variable volatile then individual threads cache its value in stack. Also updating a variable in synchronized block with update its value in all the threads storing it. do you agree with this? What you are saying will ultimately lead to this. So how come answer is wrong. – Lokesh Apr 04 '15 at 13:17
  • No, I don't agree that "updating a variable in synchronized block with update its value in all the threads storing it", this is *not* what Java Memory Model provides. I don't agree with the premise that making a variable volatile "just" exposes it to memory, and everything is fine from that moment on. This is a dangerous thinking, memory ordering is much more complicated than that. – Aleksey Shipilev Apr 04 '15 at 14:01
  • Please carefully read the example I linked: http://shipilev.net/blog/2014/jmm-pragmatics/#_happens_before_test_your_understanding. If you don't understand any piece of it, read the rest of the post. – Aleksey Shipilev Apr 04 '15 at 14:02
1

Almost the exact example is given in "JMM Pragmatics" here.

volatile provides the happens-before edges to make the changes before the setter visible to all things that happen after the getter, assuming the getter actually observed the set value. synchronized in setter additionally gives the mutual exclusion, that volatile does not guarantee alone.

Aleksey Shipilev
  • 18,599
  • 2
  • 67
  • 86
  • What do you mean by *"assuming the getter actually observed the set value"*. Does it mean the provided code in OP isn't safe? N.B. I'm a kind of new to concurrency, and need to get familiar with some basic concepts before to dig in the depth, I think. Thanks anyway for detailed info. I have to study it. – artaxerxe Apr 07 '15 at 14:23
  • What do you mean by "volatile provides the happens-before edges to make the changes before the setter visible to all things that happen after the getter"? Are you assuming a particular order of execution of setter -> getter? What does "changes before the setter" mean? – Chin May 09 '17 at 13:37
  • Answer to both questions: "assuming the getter actually observed the set value" is key here. *IF* getter observed value set by setter, then all actions before the setter happened before any action after the getter. Nothing assumes the "order of execution" here, JMM only says that *if* volatile read observed the value set by volatile write, happens-before (via synchronizes-with) is born. – Aleksey Shipilev May 09 '17 at 18:47
0

Since Integer i is the private instance variable of the class the synchronise instance method will work. You need to make sure all methods you add to class which modifies the i must synchronise using same object monitor.

Please note i++ is not automic expression so only volatile will not work.

You can use AtomicInteger for single value like this.

Himanshu Ahire
  • 677
  • 5
  • 18