views:

269

answers:

5

In the book Java Servlet Programming, there's an example servlet on page 54 which searches for primes in a background thread. Each time a client accesses the servlet, the most recently found prime number is returned.

The variable which is used to store the most recently found prime is declared as such:

long lastprime = 0;

Since this variable is begin accessed from multiple threads (the background thread that's doing the calculations and any client threads that are accessing it), doesn't it need to be declared volatile or have its access synchronized in some way?

+6  A: 

Yes, assuming you really want to see the most recently calculated prime on any thread, it should either be volatile or be accessed in a thread-safe way via synchronized blocks/methods. Additionally, as pointed out in the comments, non-volatile long variables may not be updated atomically - so you could see the top 32 bits of an old value and the bottom 32 bits of a new value (or vice versa).

I forgot about the atomicity side of things earlier because it's almost always solved automatically by when you make sure you get the most recently published value, and make sure you fully publish new values. In practice this is almost always what you want, so atomicity becomes a non-issue if your code is working properly to start with.

It's not a SingleThreadModel servlet is it? That would obviously make a difference.

Another alternative would have been to use AtomicLong.

Jon Skeet
I'd vote for AtomicLong
matt b
No, it's not a SingleThreadModel servlet. It's an example of using a servlet to do background processing.
MCS
Sounds like it's just a bad example then. It happens - authors never know what they're talking about ;)
Jon Skeet
It's not just seeing the most recent value, it's (a) seeing ANY published value -- there's no guarantee you won't see '0' forever in the reader thread, and more importantly (b) seeing a correct value at all. Updates of non-volatile longs are not atomic, you may see the bottom 32 bytes of the old value and the top 32 of the new (or vice-versa) -- which might not be a prime at all!
Cowan
True, I had neglected the atomicity side of things. Updating... (I'm not so bothered about the "any published value" bit, as that feels like it's just a generalisation of "most recent value".)
Jon Skeet
+1  A: 

Yes. A servlet's variables aren't thread-safe.

Zack
+1  A: 

There is a clean read/write split between the threads; one thread "publishes" the last prime for others to read, then you can get away with making it volatile.

If the access pattern involved some read-modify-write sequences or the like, then you'd have to synchronize the access to the field.

Christian Vest Hansen
+1  A: 

Assuming Java 5 or later then declaring it volatile gives well-defined semantics as desscribed here. On the principle of removing doubt from the code maintainer's mind I would use volatile, saying "yes I know that multiple threads use this variable".

The intersting question is the effect of not declaring it volatile. Provided that you got a prime, does it matter if it's the very latest available? Volatile ensures taht values are taken from memory, not any "CPU" caches, so you should get a more up to date value.

What about the possibility of seeing a partial assigment? Could you get really unlucky and see a long whose LSBs are part of an old value and MSBs part of a different value? Well, assignments to longs and doubles are not atomic, so in theory yes!

Ergo, volatile or synchronized is not just a nice-to-have ... you need it

djna
A: 

Semantics of volatile variable in Java are not strong enough to make the increment operation (lastprime++) atomic, unless you can guarantee that the variable is written only from a single thread - not in servlet's case

On the other hand, using AtomicXXX variables is thread-safe, as long as no compounded operations are performed. There will be window of vulnerability when updating more than one atomic variables, even though each call to is atomic.

Loop