hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dhruba Borthakur <dhr...@gmail.com>
Subject Re: volatile considered harmful
Date Thu, 15 Apr 2010 22:24:08 GMT
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 <ryanobjc@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message