I agree. The Java spec clearly mentions that load/store/read/write to volatile are atomic
http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330 <http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330>but definitely not increment and decrement. increment and decrement operations need to load *and* store data to a volatile variable and are not atomic. -dhruba On Thu, Apr 15, 2010 at 3:00 PM, Ryan Rawson <ryano...@gmail.com> wrote: > So there I was, running a unit test. I step away and hours later it > still had not finished. It was stuck on this line of the code: > > putThread.done(); > > putThread.join(); > > So the putThread was not finishing as it was supposed to do. Well, > what does putThread.done() do: > > public void done() { > done = true; > synchronized (this) { > interrupt(); > } > } > > > Looks reasonable, wait, what is this access of a variable shared among > multiple threads outside of a sync block? Oh wait, its "ok", it's > volatile: > > private volatile boolean done; > > > And what does the run loop of the thread look like? Again reasonable: > > public void run() { > done.set(false); > int val = 0; > while (!done.get()) { > // Do HRegion.put call with some exception handling > } > } > > > Well in theory this all looks reasonable. > > But why harmful? > > I had changed the Gets to not acquire row locks, thus removing a > synchronization block between the main thread (which did Gets and then > eventually called that code up top). Two consequences, one was puts > happened much faster (good!), and a negative consequence is sometimes > the test would fail to stop the put thread. For minutes and hours > after the main thread had signalled the put thread to stop, it still > had not. > > I switched the volatile boolean => AtomicBoolean and the problem went away. > > In short, the use of volatile is not guaranteed to see an update from > a different thread in any timely manner. The Java Memory Model does > not ensure this. If you need to rely on passing data between threads > you need to either use a monitor lock (using synchronized keyword > either at the method level or as an explicit block) or use one of the > concurrent utilities classes - such as AtomicLong, AtomicBoolean, or > the excellent read/write locks. > > If you wish to increment a counter from multiple threads, doing this: > volatile int count = 0; > // later: > count++; > > will not work. You must use an AtomicInt/Long or use a monitor lock. > This is because the operation "count++" is not atomic, it is actually > 3 - get, add, put. > > As we code in HBase, we should avoid the use of volatile as much as > possible. The guarantees it makes are not good enough and can cause a > lot of pain figuring this out months after you originally coded it. > -- Connect to me at http://www.facebook.com/dhruba
