Win a copy of Think Java: How to Think Like a Computer Scientist this week in the Java in General forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

Question on Volatile Variable

 
Jack Nam
Ranch Hand
Posts: 33
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Everybody,

If I access an instance of sequenceValue like below. Is it safe? should I make nextVal variable volatile?

synchronized(sequenceValue){
long newVal = sequenceValue.getNextVal();
}


class SequenceValue {

long nextval = 0;

public long getNextval() {
return nextval++;
}


}
 
Roberto Perillo
Bartender
Posts: 2271
3
Eclipse IDE Java Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hum... I don't think this question has something to do with the SCJD certification. So, let's slide this over to the Threads and Synchronization forum!

Ah, and by the way, you just have to synchronize on objects that may be accessed by more than one Thread, and you have to do this in order to protect their state. So, I'd say that this code of yours is thread-safe and you don't have to make it volatile. But, in order to always be thread-safe, you always have to synchronize on the sequenceValue object.
 
Jack Nam
Ranch Hand
Posts: 33
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I did googling on this topic, and it is kind of grey to me. Although the sequenceValue is thread-safe, but I wonder if the property is. It could be like a volatile array, not thread-safe for the element.
 
Stephan van Hulst
Bartender
Pie
Posts: 5790
61
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
No, you don't have to make it volatile. After a synchronized block exits, changes to the state of the object that was synchronized on will immediately be visible to all subsequent inspections of the object. Hence the word "synchronized".

Volatile is only needed when you access a primitive with multiple threads, through unsynchronized code.
 
Mike Simmons
Ranch Hand
Posts: 3076
13
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jack Nam wrote:Although the sequenceValue is thread-safe, but I wonder if the property is. It could be like a volatile array, not thread-safe for the element.

I would say that the code show is thread-safe if used as shown here. However nothing seems to prevent other code from calling getNextValue() in an unsafe manner, without synchronizing. It would be better to write getNextValue() so that it is always thread-safe. You can achieve this by putting the synchronization inside the method, such that anyone calling the method will activate the synchronization. This can be done with a synchronized block, or you can just synchronize the getNextValue() method itself.
 
Roberto Perillo
Bartender
Posts: 2271
3
Eclipse IDE Java Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mike Simmons wrote:I would say that the code show is thread-safe if used as shown here.


Agreed.

Mike Simmons wrote:However nothing seems to prevent other code from calling getNextValue() in an unsafe manner, without synchronizing.


Agreed, that's why I said above that "in order to always be thread-safe, you always have to synchronize on the sequenceValue object".
 
Jack Nam
Ranch Hand
Posts: 33
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ok guys.

Noticed. Thanks.
 
Anupam Sinha
Ranch Hand
Posts: 1090
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I think that the code is not thread safe.

This simple looking line



is not thread safe. Java only guarantees a 32 bit to be atomic (unless otherwise made atomic). So nextVal should be made volatile.

Apart from this I am not able to understand this program much.

I am unable to understand how this program (barring the above reason) will be thread safe (if it is made to compile).

getNextval is not synchronized so even acquiring a lock on sequenceValue shouldn't matter much.
 
Jack Nam
Ranch Hand
Posts: 33
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
if >> synchronized getNextval is thread-safe. Then the equivalent synchronized(this) inside getNextval is also safe. Then synchronized(sequenceval) is also safe since those statements are synonym.

Anupam Sinha wrote:I think that the code is not thread safe.

This simple looking line



is not thread safe. Java only guarantees a 32 bit to be atomic (unless otherwise made atomic). So nextVal should be made volatile.

Apart from this I am not able to understand this program much.

I am unable to understand how this program (barring the above reason) will be thread safe (if it is made to compile).

getNextval is not synchronized so even acquiring a lock on sequenceValue shouldn't matter much.
 
Anupam Sinha
Ranch Hand
Posts: 1090
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
What if someone accesses sequenceValue.getNextVal(); directly without the lock?
 
Jack Nam
Ranch Hand
Posts: 33
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
then it would not be thread-safe.

Anupam Sinha wrote:What if someone accesses sequenceValue.getNextVal(); directly without the lock?
 
Mike Simmons
Ranch Hand
Posts: 3076
13
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Anupam Sinha wrote:Java only guarantees a 32 bit to be atomic (unless otherwise made atomic). So nextVal should be made volatile.

Volatile will never make a ++ operation atomic, for long or any other primitive data type. The ++ operation requires that the value be read, modified, and written - that's never atomic according to volatile's rules.

I think everything else I might add has already been said several times at this point. It's thread-safe in the code here only because of the synchronized, and it's easy to (accidentally or intentionally) bypass that and make the code unsafe.
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic