Ryan: Joy had brought up a related issue to attention recently, and fwd'ed this link:
http://blogs.sun.com/dave/entry/a_race_in_locksupport_park It has to do with a volatile "_counter" variable as well. >From the above writeup: <<< The problem would only manifest when we were using the -UseMembar optimization that lets us remove fences from certain hot thread state transitions paths that need to coordinate safepoints between mutator threads and the JVM. This feature is enabled by default, but we can turn it off with the -XX:+UseMembar switch, which causes the JVM to emit normal fence instructions in the state transitions paths. (That particular optimization is an example of asymmetric Dekker synchronization). Crucially, the park() path contains such a state transition. In reality the fence emitted by the +UseMembar switch was simply covering up the otherwise latent Parker:: bug. +UseMembar constitutes a work-around. Sensitivity to UseMembar was initially confounding but ultimately a valuable clue.>>> -----Original Message----- From: Dhruba Borthakur [mailto:dhr...@gmail.com] Sent: Thursday, April 15, 2010 3:24 PM To: hbase-dev@hadoop.apache.org Subject: Re: volatile considered harmful 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
