Question on Volatile Variable

[Jack Nam]

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]

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]

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]

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]

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]

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]

Ok guys.

Noticed. Thanks.

[Anupam Sinha]

I think that the code is not thread safe.

This simple looking line

long nextval = 0; 

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]

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

long nextval = 0; 

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]

What if someone accesses sequenceValue.getNextVal(); directly without the lock?

[Jack Nam]

then it would not be thread-safe.

Anupam Sinha wrote:What if someone accesses sequenceValue.getNextVal(); directly without the lock?

[Mike Simmons]

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.